-
Notifications
You must be signed in to change notification settings - Fork 43
Create Pipes - Kate Evans-Spitzer - Solar System.rb #47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Solar SystemWhat We're Looking For
Great work overall! I've got a few nitpicks pointed out below, but in general I'm quite happy with what you've submitted. |
| def initialize(name, planets) | ||
| @system_name = name.to_s | ||
| @planets = {} | ||
| planets.each {|new_planet| @planets[new_planet.name] = new_planet} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nitpick: in general, you should expand your each loops into multiple lines. So in this case that would be:
planets.each do |new_planet|
@planets[new_planet.name] = new_planet
end|
|
||
| def add_planet(*planet) # Adds one or more Planet objects to solar system | ||
| planet.each {|new_planet| @planets[new_planet.name] = new_planet} | ||
| @num_planets = @planets.length |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's interesting to me that you stored the number of planets in a variable. As we can see, you have to remember to update the value every time the set of planets changes. This isn't necessarily a bad thing, but it's an extra piece for you the developer to remember. For example, it would be easy to write a remove_planet method and forget to update the count.
A cleaner way to do it might be to build an instance method that provides direct access to @planets.length, something like:
def num_planets
return @planets.length
endThis has the same effect, without requiring you to keep track of an extra thing.
| def distance_between(first, second) | ||
|
|
||
| if first.class == String # DRY??? | ||
| first = @planets[first] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might be able to write a method do do this work.
Solar System
Congratulations! You're submitting your assignment.
Comprehension Questions
initializemethod in your class?SolarSystemused anArrayvs aHash.