-
Notifications
You must be signed in to change notification settings - Fork 45
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
Create ss_wave_2.rb #29
base: master
Are you sure you want to change the base?
Conversation
Solar SystemWhat We're Looking For
|
def pick_planet(user_pick) | ||
@ss.each do |planets| | ||
if planets.planet_name == user_pick | ||
puts "#{planets.details}\n\n\n#{planets.planet_attributes}" |
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.
I love the idea of this method! One way you might make it more flexible is to return the Planet
itself, rather than puts
ing a string. That way, the caller could do whatever they like with it.
if solar_system.planet_list.include?(planet_choice) | ||
return solar_system.pick_planet(planet_choice) | ||
else | ||
while solar_system.planet_list.include?(planet_choice) == false |
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.
The conditions for the if
and the while
here are a little redundant. Maybe this code could be refactored to something like:
planet_choice = nil
until solar_system.planet_list.include?(planet_choice)
puts "Please choose a planet"
planet_choice = gets.chomp.downcase.capitalize
end
solar_system.pick_planet(planet_choice)
planet_choice = gets.chomp.downcase.capitalize | ||
return solar_system.pick_planet(planet_choice) | ||
|
||
end |
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.
Since you have a return in your while
loop here, the loop body will only ever execute once!
|
||
def initialize(planet_1, planet_2 ) | ||
@ss = [planet_1, planet_2] | ||
|
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.
While this certainly works, we were expecting you to pass a list of planets directly into SolarSystem.new
, rather than passing planets one by one. So something like:
class SolarSystem
attr_accessor :ss
def initialize(planets)
@ss = planets
end
# ... rest of the class ...
end
planet_a = Planet.new("Jupiter","1882","details")
planet_b = Planet.new("Uranus", "1932","about")
planet_list = [planet_a, planet_b]
my_ss = SolarSystem.new(planet_list)
That way whoever creates the instance of SolarSystem
can create it with as many planets as needed.
if user_create == "y" || user_create == "yes" | ||
user_planet_name | ||
user_planet = gets.chomp | ||
puts "when was your planet discovered?" |
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.
This code is almost exactly the same as the planet creation code you've got starting on line 166. Could you consolidate the two somehow?
Solar System
Congratulations! You're submitting your assignment.
Comprehension Questions
initialize
method in your class?SolarSystem
used anArray
vs aHash
.