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

Katrina - Edges - Solar System #28

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

Conversation

kaganon
Copy link

@kaganon kaganon 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? The initialize method is a constructor that is run when we create a new instance of a class. It tells us what properties the class has, and that they are available to use within the class.
Why do you imagine we made our instance variables readable but not writable? I'd imagine it's because the properties that we assigned to the instance variables are states that we do not expect to change (e.g., name).
How would your program be different if each planet was stored as a Hash instead of an instance of a class? If my data were stored in a hash, I would have to write more repetitive code. What comes to mind is creating a new instance of a planet. If I needed to create a new planet, I would have to go through and assign each key/value pair to a new hash vs using the class method .new to create a new instance of a planet.
How would your program be different if your SolarSystem class used a Hash instead of an Array to store the list of planets? If for some reason I had two instances of a planet with the same name, and assuming my hash has planet name as the key, I would only be able to keep one planet when I push the new planet into my list of planets. I would also have to change how I iterate over my list of planets.
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 planet and solar system classes each have specific responsibilities. The planet class holds all the information regarding details only about a planet, while the solar system class holds the information for each of the planets and also different functions that determines relationship between two planets (e.g. distance between two planets).
How did you organize your require statements? Which files needed requires, and which did not? What is the pattern? My solar system file used require_relative 'planet', and my main file used require_relative 'planet' and require_relative 'solar_system'. Having classes in separate files makes it easier to access the codes without having to copy/paste a whole long block of code into one file. Overall, it makes it more organized and mobile.

@tildeee
Copy link

tildeee commented Aug 31, 2018

Solar System

What We're Looking For

Feature Feedback
Baseline
Whitespace (indentation, vertical space, etc) x
Variable names x
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 x
Overall

Good work on this project!

The code looks good, the overall project looks good -- the code is readable, clean, and hits all the requirements.

I think you put a lot of attention to detail in this project, especially with considering things like gets.chomp.strip, etc.

Also, great job with handling exceptions!

I'm making a few comments on one bug that I see, but otherwise the project looks great, great job!

end
list = solar_system.list_planets

if user_input == 'A'
Copy link

Choose a reason for hiding this comment

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

Instead of checking what user_input is equal to with a bunch of if/elsif/else clauses, what else could we do?

first_user_planet_found = solar_system.find_planet_by_name(user_first_planet)
second_user_planet_found = solar_system.find_planet_by_name(user_second_planet)

if first_user_planet_found && second_user_planet_found
Copy link

Choose a reason for hiding this comment

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

Handling the idea of "what if a planet isn't found" wasn't a requirement, so I usually don't comment on this, but here you are deliberately checking to see if the planets were found so I wanted to chime in...

Your SolarSystem#find_planet_by_name method actually will never return nil! Therefore, this line of code if first_user_planet_found && second_user_planet_found will never be found. Instead, if your method find_planet_by_name doesn't find a planet, it will return the @planets array.

# Welcome message to user
def welcome_message
puts "=" * 80
puts "\t\t\tHello and Welcome to THE MAGIC SCHOOL BUS!"
Copy link

Choose a reason for hiding this comment

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

🚌

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