Skip to content
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

Amber Lynn - Edges - Solar-System #39

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

griffifam
Copy link

Solar System

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Answer
When does the initialize method run? What does it do? When a method is called
Why do you imagine we made our instance variables readable but not writable? Because we don't plan to change them at any point and don't want anyone else to either.
How would your program be different if each planet was stored as a Hash instead of an instance of a class? They would have less functionality
How would your program be different if your SolarSystem class used a Hash instead of an Array to store the list of planets? The data inside would not behave the way I'd want.
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? One class is responsible for creating planets. The other is responsible for taking in and storing planets in an array.
How did you organize your require statements? Which files needed requires, and which did not? What is the pattern? planet.rb and solar system.rb that hold my classes. Classes should have their own file and we can require them in our main file to bring it all together.

@droberts-sea
Copy link

Solar System

What We're Looking For

Feature Feedback
Baseline
Whitespace (indentation, vertical space, etc) yes
Variable names yes
Git hygiene yes - good work
Planet
initialize method stores parameters as instance variables with appropriate reader methods yes
summary method returns a string yes
SolarSystem
initialize creates an empty list of planets yes
add_planet takes an instance of Planet and adds it to the list yes
list_planets returns a string yes
find_planet_by_name returns the correct instance of Planet not quite - see inline
CLI
Can list planets and quit yes
Can show planet details yes
Can add a planet yes
Complex functionality is broken out into separate methods yes
Overall Good work overall! This code is well-organized and easy to read, and does a good job of solving the problem at hand. I've left a few inline comments for you to review, but it's clear the learning goals of the assignment were met. Keep up the hard work!

elsif user_input == "3"
puts "Name a planet: "
user_planet_name = gets.chomp
puts "Color: "

Choose a reason for hiding this comment

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

This work is complex enough it would probably make sense to put it in a separate method.

@planets.length.times do |i|
if planet_name.downcase == @planets[i].name.downcase
# matching_planet = @planets[i]
#cant figure out how to call summary method

Choose a reason for hiding this comment

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

This definitely solves the problem, but we could clean this code up a little by using an enumerable:

def find_planet_by_name(planet_name)
  planet_name = planet_name.downcase
  return @planets.find { |planet| planet.name.downcase == planet_name }
end

end
end
return x.summary
end

Choose a reason for hiding this comment

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

Instead of returning the summary here, you should return the planet itself. Then whoever calls this method can do what they want with it. That might be printing the summary, comparing it with another planet, etc.

Choose a reason for hiding this comment

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

Also, I think this will not quite work if the planet is not found.

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