Skip to content

Branches - Macaria#33

Open
mdove92 wants to merge 16 commits intoAda-C12:masterfrom
mdove92:master
Open

Branches - Macaria#33
mdove92 wants to merge 16 commits intoAda-C12:masterfrom
mdove92:master

Conversation

@mdove92
Copy link

@mdove92 mdove92 commented Aug 21, 2019

Solar System

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Answer
When does the initialize method run? What does it do? Initialize takes in any parameters for a given class and creates an instance of said class.
Why do you imagine we made our instance variables readable but not writable? We don't want our instance variables to be changed after creating an instance. The attributes of an instance of a planet should be unchanging.
How would your program be different if each planet was stored as a Hash instead of an instance of a class? Every method would require enumeration. It would be more difficult to access and format data of the instance/planet. Instead of instance variables we would have to use key-value pairs.
How would your program be different if your SolarSystem class used a Hash instead of an Array to store the list of planets? The program would be less readable. Iterating over the hash would require many more lines of code than using instance variables.
There is a software design principle called the SRP. The Single Responsibility Principle (SRP) says that each class should be responsible for exactly one thing. Do your classes follow SRP? What responsibilities do they have? I think that my classes follow SRP. A solar system does not define anything other than that object, despite it containing multiple children objects, planets.
How did you organize your require statements? Which files needed requires, and which did not? What is the pattern? All of my 'require' statements are at the top of my files. Whichever was used in the code first was listed first. Main.rb required access to both class files, as it called on methods specific to those classes. Solar_system.rb required planets, as the an instance of a solar system class is made up of instances of planets.

@tildeee
Copy link

tildeee commented Sep 3, 2019

Solar System

What We're Looking For

Feature Feedback
Baseline
Whitespace (indentation, vertical space, etc) x
Variable names x
Git hygiene x -- It seems like your git commits are being made with an email address (configured in your Terminal) that GitHub is confused about. Can you confirm that the email address configured for git in your terminal matches what's on your github account, and then ask me for help if in the next project it continues to seem weird?
Planet
initialize method stores parameters as instance variables with appropriate reader methods x
summary method returns a string Nope :(
SolarSystem
initialize creates an empty list of planets x
add_planet takes an instance of Planet and adds it to the list x
list_planets returns a string x
find_planet_by_name returns the correct instance of Planet x
CLI
Can list planets and quit x
Can show planet details x
Can add a planet x
Complex functionality is broken out into separate methods x
Overall

Good work on this project, Macaria! Your code was logical, thoughtful, and well-organized. In particular, you did a FANTASTIC job of creating helper methods and paying close attention to detail-- I can tell from all of the edge cases that you coded for.

I've added a few comments about tiny suggestions for improvement.

Overall, well done! Great work!


# Format attributes of instances od planets into a neat string.
def summary
puts "The planet #{@name} is #{@color} in color and is #{"%1e" % @mass_kg} kgs.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job with always making sure that the numbers are formatted!


# Format attributes of instances od planets into a neat string.
def summary
puts "The planet #{@name} is #{@color} in color and is #{"%1e" % @mass_kg} kgs.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our requirements asked for the summary method to return a string, not puts it, and to shift the responsibility of putsing things to the main method. :(

if planet.name.downcase == planet_to_find.downcase
return planet
end
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you had more time to refactor, consider refactoring this each loop with a find

return planet
end
end
return nil_planet = Planet.new(name: "", color: "", mass_kg: 1, distance_from_sun_km: 1, fun_fact: "")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting/clever solution to return an empty Planet!

Also, in this line, you are doing a few things: Creating a new instance of Planet, assigning it to the local variable nil_planet, and then returning that value. It would make the same amount of sense without the variable assignment, so, return Planet.new(...)

exploder = Planet.new(name: "Exploder", color: "fire", mass_kg: 1.847e26, distance_from_sun_km: 1.0826e5, fun_fact: "Exploder is too close to the Sun. It is going to explode.")
untenable.add_planet(poddo)
untenable.add_planet(normie)
untenable.add_planet(exploder)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cute and good system and planets ⚡

while untenable.find_planet_by_name(user_planet).name != ""
puts "A planet with that name already exists. Try and be ORIGINAL."
user_planet = get_string.downcase
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work checking for this use case! This is a great edge case to account for

end

# Helper method to obtain user input of planet names and use find_planet_by_name(solar_system.rb) to validate it
def get_planet(solar_system)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIIIIIICE! :) All of these helper methods are soooo good and so necessary!

planet = gets.chomp.downcase
planet_select = solar_system.find_planet_by_name(planet)

while planet_select.name.downcase != planet
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you didn't always return a new instance of Planet from the find_planet_by_name method in cases where there was no matching planet, this conditional would be different and you wouldn't have to check planet_select.name.downcase. For example, if that method returned nil when it didn't find any planet, then this while could look like while planet_select == nil (or even shorter: while !planet_select)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants