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

Solar System - Val - Edges #31

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

Conversation

valgidzi
Copy link

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 method's class is created.
Why do you imagine we made our instance variables readable but not writable? The variables could be easily reassigned to new values if they were also writable, and these values need to be fixed.
How would your program be different if each planet was stored as a Hash instead of an instance of a class? If each planet were stored as a 'Hash', I wouldn't be able to use the methods in this program to access the data.
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' instead of an 'Array', the methods that populate the array and iterate over it would need to be updated.
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? My classes follow SRP. The Planet class creates new instances of Planet and their instance variables. The SolarSytem class stores and manages information about Planets in SolarSystems.
How did you organize your require statements? Which files needed requires, and which did not? What is the pattern? In main.rb my program 'require's planets and solar_systems. The file where the methods are called needs 'require', the other files only define methods and don't need 'require'.

@tildeee
Copy link

tildeee commented Aug 28, 2018

Solar System

What We're Looking For

Feature Feedback
Baseline
Whitespace (indentation, vertical space, etc)
Variable names
Git hygiene x
Planet
initialize method stores parameters as instance variables with appropriate reader methods x
summary method returns a string x
SolarSystem
initialize creates an empty list of planets x
add_planet takes an instance of Planet and adds it to the list x
list_planets returns a string x
find_planet_by_name returns the correct instance of Planet x
CLI
Can list planets and quit x
Can show planet details x
Can add a planet x
Complex functionality is broken out into separate methods complex functionality was turned into separate methods in SolarSystem-- it may make more sense to put this into main
Overall

Hi Val! Great job on this project

The code looks good and is well-written and well-formatted. It hits all the requirements. The code shows clear understanding of what each class has, and the methods (behavior) all make sense.

You end up putting into SolarSystem some methods that ask for input, such as planet_details, and new_planet. It may make sense for these methods to live in main.rb, and then the parts that affect an instance of SolarSystem can be called from there, such as add_planet(name, color, mass, distance, fun_fact) or find_planet_by_name(planet). This will promote the responsibilities of different classes-- SolarSystem can just focus on being a good Solar System, while main will be concerned with asking the user for input and reading/parsing the input.

Otherwise, overall, good work! Looks great.

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