Skip to content

Branches-Steph_Garcia#47

Open
Steph0088 wants to merge 3 commits into
Ada-C12:masterfrom
Steph0088:master
Open

Branches-Steph_Garcia#47
Steph0088 wants to merge 3 commits into
Ada-C12:masterfrom
Steph0088:master

Conversation

@Steph0088

Copy link
Copy Markdown

Solar System

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Answer
When does the initialize method run? What does it do? When you create a new planet object.
Why do you imagine we made our instance variables readable but not writable? They don't have to be changed once they are created.
How would your program be different if each planet was stored as a Hash instead of an instance of a class? They might have been easier to work with and then you could have changed the find planet method and even changed the list planet name method to be simpler and just have called on the value you needed by key.
How would your program be different if your SolarSystem class used a Hash instead of an Array to store the list of planets? I think you would have to have been more specific when retrieving planet details because you would have to use the key.
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, my program does do exactly one thing. They are simple and pretty straightforward.
How did you organize your require statements? Which files needed requires, and which did not? What is the pattern? The main needed the require statements because it needed the two classes in order to function. In the main you call on class methods from planets and solar system.

@tildeee

tildeee commented Aug 30, 2019

Copy link
Copy Markdown

Solar System

What We're Looking For

Feature Feedback
Baseline
Whitespace (indentation, vertical space, etc) x
Variable names x
Git hygiene x, don't forget to keep up your git workflow habits :)
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, there's a tiny bug that I've written about below
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 there is opportunity to refactor repeated code into methods outside of the main method
Overall

Hi Steph! Good work on this project. Your code here to practice object-oriented program is exactly what we were looking for-- great job on making Planet use instance variables and have the instance method summary, and great job with creating SolarSystem with its more complex functionality.

There happen to be a few bugs that occur with the main method in case the user puts in some invalid input. Unfortunately, these bugs end up breaking the user flow in some cases, which is really unfortunate :( I've added some details around the code to talk about these bugs.

Besides the user flow breaking, I feel really confident about the object-oriented code here. I think that if you had more time to work out the bugs in the main method around the logic in loops, variable assignment, and return statements, this would have been a perfect submission.

That being said, good work; let me know if you have questions!

Comment thread lib/planet.rb
@fun_fact = fun_fact

if @mass_kg <= 0
raise ArgumentError.new "Invalid input must be greater than 0"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

When I put in a user input of 0 or 9banana for the mass, my program raises an error here and breaks! This is good for programmers, but in terms of being a user, this is really weird to look at.

As a user, I probably would prefer if the main method told me to re-input my mass in kg if I put in bad input.

That being said, I think your Planet class code looks correct, and it should stay the same :) This way, the error will only be raised if we do something weird, like if we made unit tests that did the wrong thing.

Comment thread lib/solar_system.rb
def find_planet_by_name(input_planet)
@planets.each do |each_planet|
if input_planet.downcase == each_planet.planet_name
return each_planet

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What happens if the input_planet name is never found in the array of @planets? If input_planet is found (like earth), then this each loop will eventually run return each_planet. However, what happens if line 25 is never true?

What happens is a very weird Ruby thing: Ruby will implicitly return the array @planets.

You have three options here to "fix" this: You can either change this find_planet_by_name method to return something special if the planet is never found, OR you change main so that if sun.find_planet_by_name doesn't get a Planet instance back, then it does something special... OR you can do both. (It's likely you'd have to do both)

Comment thread main.rb
puts "Here are the planets in your SolarSystem:"
puts sun.list_planets
puts "Would you like more information about one of to the planets?"
user_response = gets.chomp

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hm, I don't think this re-assignment of user_response gets used ...

Comment thread main.rb
puts "Which one would you like more information about?"
user_choice_planet = gets.chomp.to_s.downcase
found_planet = sun.find_planet_by_name(user_choice_planet)
puts found_planet.summary

@tildeee tildeee Aug 30, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If the program asks me "Which one would you like more information about?" and I respond 9banana, I get an error here: undefined method 'summary' for #<Array:0x00007fb5e0077a40> (NoMethodError). This is because here we are calling found_planet.summary, meaning found_planet was an Array in this case. This line works when found_planet is an instance of Planet. To find out why found_planet was an array (instead of an instance of Planet), I would look at the line above, found_planet = sun.find_planet_by_name(user_choice_planet)... I'm leaving a comment on SolarSystem's find_planet_by_name

Comment thread main.rb

#Where I allow user to add another planet:
puts "Would you like to add you own planet to the sun Solarsystem?"
user_response = gets.chomp.to_s.downcase

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your code is set up for main to run over and over again as long as the user says yes to some prompt that would ask if the user wants to re-run the Solar System program (I know this from your while user_response == 'yes' line!).

However, because you re-assign user_response here (and above), your while loop won't loop because user_response may change here.

This is a shame because then your program overall exits after you add a planet!

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