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

Sockets - Grace #40

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

Sockets - Grace #40

wants to merge 27 commits into from

Conversation

gracemshea
Copy link

Solar System

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Answer
When does the initialize method run? What does it do? Creates a blueprint for all instance variable you want
Why do you imagine we made our instance variables readable but not writable? Because you wanted to grant access without the risk of overwriting or changing the data
How would your program be different if each planet was stored as a Hash instead of an instance of a class? Well, according to Dee's lecture, we wouldn't be able to store behaviors just put data in and get it out
How would your program be different if your SolarSystem class used a Hash instead of an Array to store the list of planets? Um I'd have to rewrite my code to reference a key/value pair. I'm imagining it like the drivers and driver info from rideshare. Please don't make me do that.
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, I think this follows that principle. The Solar_System holds the collection of planets, Planet holds the planets' information.
How did you organize your require statements? Which files needed requires, and which did not? What is the pattern? I only put require statements in main. I see it as kind of the center and the other files branch off of it.

@droberts-sea
Copy link

Solar System

What We're Looking For

Feature Feedback
Baseline
Whitespace (indentation, vertical space, etc) yes
Variable names OK, but some of your method names are a little awkward
Git hygiene yes
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 almost - see inline
CLI
Can list planets and quit yes
Can show planet details no - see inline
Can add a planet no - see inline
Complex functionality is broken out into separate methods yes
Overall

This is a good start - your code is well-organized, and it's clear that the learning goals around creating and working with classes were met. Unfortunately some of the functionality of this app is broken as-submitted. In both cases all that was needed to fix it was a small change, indicating that you probably changed things at the last minute and submitted without testing it out.

I'm also seeing some confusion around the difference between puts and return. In general, the methods in our classes should always return, and interacting with the user (puts and gets.chomp) should be isolated to driver code like main.rb.

That being said, I'm not too concerned by what I see here. Make sure to test your code before you turn it in, and keep up the hard work!

def find_planet_by_name(planet)
@planets.each do |planet|
if planet.name == name.capitalize
return planet

Choose a reason for hiding this comment

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

This code has a bug! Your parameter (which should be the name of a planet) is called planet, but you override this on line 47 by using the same name for your iteration parameter. The on line 48, you look for the variable name, which doesn't exist.

Probably the parameter should be called name, not planet. Making this change seems to fix the error.

Copy link
Author

Choose a reason for hiding this comment

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

Dan, thank you for your feedback! I will take a look at this over the weekend and refactor.

when input == "3"
solar_system.add_a_planet
puts "\n...Entered into system. Thank You."
when input == "4"

Choose a reason for hiding this comment

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

Option 3 does not work - the method on SolarSystem is get_info_new_planet, not add_a_planet.

def get_info_new_planet
print "\nWhat is your new Planet's name?"
planet_name = gets.chomp.to_s
print "\nWhat is #{planet_name}'s color?"

Choose a reason for hiding this comment

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

You have a lot of code that interacts with the user here, which makes me think this method might fit better in main.rb.

planet1 = get_planet_name
planet2 = get_planet_name
distance = planet1.distance_from_sun_km - planet2.distance_from_sun_km
puts "\nDistance between #{planet1.name} and #{planet2.name}: #{distance.abs} km."

Choose a reason for hiding this comment

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

You should return a number here instead of putsing the result. Then whoever called this method could use that number. For example, if someone wanted to write a method to find the two planets that are closest together, a number would be much more valuable than this string.

def get_planet_name
print "\nEnter planet name: "
planet = gets.chomp.to_s
return find_planet_by_name(planet)

Choose a reason for hiding this comment

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

This method also might fit better in main.rb

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