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 - Elizabeth #48

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

Leaves - Elizabeth #48

wants to merge 12 commits into from

Conversation

north108
Copy link

Solar System

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Answer
When does the initialize method run? What does it do? It runs in planet.rb. It is used to make a new instance of class.
Why do you imagine we made our instance variables readable but not writable? Because we don't want a user to be ale to change the instances.
How would your program be different if each planet was stored as a Hash instead of an instance of a class? It would be a lot longer and all on one page of code.
How would your program be different if your SolarSystem class used a Hash instead of an Array to store the list of planets? Every piece of information would be stored in a key and it would make a long list of hashes in solar_system.
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 because it's only responsibility is to hold planet information. However, my SolarSystem does a few things, including listing planets and creating new planets.
How did you organize your require statements? Which files needed requires, and which did not? What is the pattern? I didn't have any require statements.

@dHelmgren
Copy link

Solar System

What We're Looking For

Feature Feedback
Baseline
Whitespace (indentation, vertical space, etc) good
Variable names good
Git hygiene Great!
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 comments. I would encourage you to group them with the CLI component.
Overall Solid job overall! It's clear to me that the learning goals on this assignment were met. Take a peek at my comments to see what can be improved, and keep up the hard work!

play = gets.chomp
puts

while play == "y"

Choose a reason for hiding this comment

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

Using this syntax is almost the same as while true. since play never gets changed, this looks like an infinite loop, which is not so good.

end

def planet_details
print "Enter the name of the planet you wish to learn about: "

Choose a reason for hiding this comment

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

Again, this probably belongs in main.rb

puts
when "2"
unknown_planet = solar_system.planet_details
p unknown_planet

Choose a reason for hiding this comment

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

p prints out some extra quotes. It looks weird!


def new_planet
print "Planet's name: "
new_name = gets.chomp

Choose a reason for hiding this comment

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

To keep the solar_system at a single responsibility, I would move this communication with the user into your main.rb, perhaps in a helper method.

end

def summary
return "#{self.name} is a #{self.color} planet that weighs #{mass_check(self.mass_kg)} kg and is #{distance_check(self.distance_from_sum_km)} km from the sun. Fun fact: #{self.fun_fact}."

Choose a reason for hiding this comment

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

This is a weird place to call distance_check and mass_check.

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