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

Daniela Sanchez - Leaves #30

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

Daniela Sanchez - Leaves #30

wants to merge 4 commits into from

Conversation

dnsanche
Copy link

@dnsanche dnsanche 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 allows us to give/require initial data to our class.
Why do you imagine we made our instance variables readable but not writable? Because we didn't want someone updating them later. The information of the planets shouldn't change.
How would your program be different if each planet was stored as a Hash instead of an instance of a class? If each planet would be stored as a hash, I would need to create keys to store each planet detail, and then when I would want to call the planet, I would need to put the name of the hash and the key I want to access to get the value. Is it more readible to use instances of classes.
How would your program be different if your SolarSystem class used a Hash instead of an Array to store the list of planets? Since the planets were stored in an array, I had to know the index of each planet to get the information when the user input was different from a planet name. With the hash, I will need to find the hash name and then, call the key to print the value. It may be more intuitive to use keys than to know the index in the array.
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? Yes, each class has the responsibility of creating an intance of the class and they have specific methods associated only to the class that do a single function.
How did you organize your require statements? Which files needed requires, and which did not? What is the pattern? The main.rb file needs to have access to the other files to have access to the classes.

@dHelmgren
Copy link

Solar System

What We're Looking For

Feature Feedback
Baseline
Whitespace (indentation, vertical space, etc) Good
Variable names good
Git hygiene I'd like to see more commits on a project of this size!
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 no, it returns an array.
find_planet_by_name returns the correct instance of Planet Mostly, see comment.
CLI
Can list planets and quit yes, but see comment.
Can show planet details yes
Can add a planet yes
Complex functionality is broken out into separate methods yes
Overall This project is pretty solid. Most of your logic is correct, but there are a few requirements that got skipped or incorrectly interpreted. I've outline what can be improved in my comments, so read those and keep up the hard work!

end

valid_input = false
until valid_input

Choose a reason for hiding this comment

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

So, the idea of the control loop here is to let the user do one action, and then another, and then another, until they choose the exit option. If we only do the first action they take, we can't see the wonderful planets that they add.

def find_planet_by_name(planet_name)
i=0
@planets.each do |planet|
if planet.name == planet_name

Choose a reason for hiding this comment

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

our instructions were looking for the match to be case irrelevant, ie "earth" should match with "EaRTH". How could you achieve this?

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