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

Leaves_Ga-Young #35

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

Leaves_Ga-Young #35

wants to merge 7 commits into from

Conversation

gyjin
Copy link

@gyjin gyjin 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? The initialize method runs when a new instance of that class is being made. It is creating a new instance of that class.
Why do you imagine we made our instance variables readable but not writable? We probably don't want the user to be able to change data that is already hard-coded into the program, like the already existing Sol solar system and the two planets.
How would your program be different if each planet was stored as a Hash instead of an instance of a class? I believe this program could have been completed in one file if planets were stored in hashes. I feel like it would be harder to keep track of or add new planets with hashes. It also would be difficult to add a new solar system.
How would your program be different if your SolarSystem class used a Hash instead of an Array to store the list of planets? If planets were stored in a hash, making them a class would be unnecessary. All the information for each planet would probably be stored in that hash. It would make accessing/searching that data different.
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 my Planet class does follow SRP - it's responsible for making a new instances of Planet and printing an easy-to-read summary of them. However, my SolarSystem class contains a lot of helper methods between the solar-system and planets. I'm not sure if this follows SRP.
How did you organize your require statements? Which files needed requires, and which did not? What is the pattern? Because main.rb referenced both the planet class and solar-system class, both planet.rb and solar_system.rb were required. Planet.rb didn't reference any other classes or files, so that did not require any 'require's. Solar_system.rb also didn't need any 'require's because all of the planet information was being added to the solar-system class.

@dHelmgren
Copy link

Solar System

What We're Looking For

Feature Feedback
Baseline
Whitespace (indentation, vertical space, etc) yes
Variable names yes
Git hygiene yes
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 yes
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, but see comment
Overall Good work over all! I can tell the learning goals for this assignment were met, but there is a quick comment worth taking a look at. Keep up the hard work.


# method to add a new planet
def add_new_planet
puts "\nWhat is the name of this new planet?"

Choose a reason for hiding this comment

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

In the interest of keeping things to a single responsibility, I would move the methods that do any sort of puts or chomp into main.rb

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