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

Trang Frego - SolarSystem #33

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

Trang Frego - SolarSystem #33

wants to merge 6 commits into from

Conversation

tfrego
Copy link

@tfrego tfrego commented Aug 23, 2018

Solar System

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Answer
When does the initialize method run? What does it do? It runs in the Class block of code to set instance variables.
Why do you imagine we made our instance variables readable but not writable? We made it readable so we can access the instance variables in Main.rb but made it not writable so that the data is not overwritten.
How would your program be different if each planet was stored as a Hash instead of an instance of a class? My program would be one large file without the flexibility of breaking them into individual files and calling methods in those files.
How would your program be different if your SolarSystem class used a Hash instead of an Array to store the list of planets? The SolarSystem class could contain all the information of each planet within the hash.
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? Yes, the SolarSystem class is responsible for only information pertinent to its solar system which is the star and planets. The Planet class has information specific to the individual planets.
How did you organize your require statements? Which files needed requires, and which did not? What is the pattern? I organized my 'require' statements with the needed classes first then the gems. The files needed depended on whether I was using the classes and gems in the following code.

@CheezItMan
Copy link

Solar System

What We're Looking For

Feature Feedback
Baseline
Whitespace (indentation, vertical space, etc) Check
Variable names Check
Git hygiene Good number of commits and good commit messages
Planet
initialize method stores parameters as instance variables with appropriate reader methods Check
summary method returns a string Check
SolarSystem
initialize creates an empty list of planets Check
add_planet takes an instance of Planet and adds it to the list Check
list_planets returns a string Check
find_planet_by_name returns the correct instance of Planet Check
CLI
Can list planets and quit Check
Can show planet details Check
Can add a planet Check
Complex functionality is broken out into separate methods Good, but it could be broken up more.
Overall Very nice work, you hit all the learning goals and the distance extra. Great work!

return planet_details
end

def main

Choose a reason for hiding this comment

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

You can probably break up this method a bit more

@color = color
if mass_kg <= 0
raise ArgumentError, 'Mass must be greater than 0'
else

Choose a reason for hiding this comment

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

You don't need the else, the raise will exit the method anyway

planet_name.downcase!
planet_name.capitalize!

planet = @planets.select { |planet| planet.name == planet_name }

Choose a reason for hiding this comment

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

You could also use .find which returns the first one that matches and nil if nothing matches.

end

def distance_between(planet1, planet2)
two_planets = @planets.select { |planet| planet.name == planet1 || planet.name == planet2 }

Choose a reason for hiding this comment

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

Nice! 🥇

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