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

Danielle Metzner - Solar System #36

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

Conversation

danimetz
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 at the beginning of each class (planets & SolarSystem) It determines what is needed to create the class.
Why do you imagine we made our instance variables readable but not writable? We don't want the user to be able to access the planet once it's initially created to change any of the characteristics of the planets.
How would your program be different if each planet was stored as a Hash instead of an instance of a class? Instead of instance variables we would have the "name", "color", etc. as keys and the details as the values.
How would your program be different if your SolarSystem class used a Hash instead of an Array to store the list of planets? You would need to know the planet name to access the info.
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 class only creates the planets. The solar system accesses the planets that are stored and retrieves their information
How did you organize your require statements? Which files needed requires, and which did not? What is the pattern? Only main.rb and spec required the different class files. I always organized the requires at the top of the file, with the more specific one first (planets).

@CheezItMan
Copy link

Solar System

What We're Looking For

Feature Feedback
Baseline
Whitespace (indentation, vertical space, etc) Well done
Variable names Nice
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 Nice
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 Nope!
Overall Well done, you hit all the learning goals for the project. Excellent work. I did leave a few minor comments in your code, but you did very well!

#Calculates the distance between two planets
def distance_between(planet1, planet2)
distance = find_planet_by_name(planet2).distance_from_sun_km - find_planet_by_name(planet1).distance_from_sun_km
return distance.abs

Choose a reason for hiding this comment

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

nice 🥇

@planets = []
end

attr_reader :star_name, :planets

Choose a reason for hiding this comment

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

It's convention to put the attr_reader and attr_accessor etc above initialize and below the class name

require_relative 'planet'
require_relative 'solar_system'

def main

Choose a reason for hiding this comment

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

This method is just crying to be broken into smaller methods!


#Finds the planet with the same name and returns the planet object
def find_planet_by_name(planet_name)
found_planet = @planets.select {|planet| planet.name.downcase == planet_name.downcase}

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 instead of .select

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