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

Tor Shimizu - Solar System - Octos #23

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

Conversation

torshimizu
Copy link

Solar System

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Answer
What was the purpose of the initialize method in your class? The initialize method would be automatically called each time a new instance of the class was created. For the Planet class, it would save the name argument as an instance variable, for the Solar System class, it would save some instance variables and call some methods
Describe an instance variable you used and what you used it for. I made a planet_hash that would have a number as a key and the planet name as the value. This hash would be used to select the summary to display in another Class method.
Describe what the difference would be if your SolarSystem used an Array vs a Hash. I would have to iterate through the input differently.
Do you feel like you used consistent formatting throughout your code? I do feel like my indentation was correct and consistent, I feel as if my code could have been made more DRY, I would use the same types of methods/control flow for the same actions in a different menu.

@droberts-sea
Copy link

Solar System

What We're Looking For

Feature Feedback
Baseline
Readable code with consistent indentation. yes
Primary Requirements
Created Custom Solar System Class with initialize, add planet & list planets methods, without using puts. yes
Planet Class Created yes
Created a collection of Planet objects as an instance variable in SolarSystem. yes
Accessor methods created yes
Method created to return the Planet's attributes and not use puts yes
Created a user interface to interact with the SolarSystem including adding a planet and viewing a planet's details yes
Additional Notes Great job overall! I have a couple of inline comments below that you should check out, but in general your code is well-organized and easy to read, and I am quite happy with this submission. Keep up the hard work!

def create_planet_hash # this method ties the index +1 of the planet to the instance of the planet, for selection in the menu
# @planet_hash = Hash[@planet_names.zip(@planets)]
@planets.each.with_index(1) do |planet, index|
@planet_hash[index] = planet

Choose a reason for hiding this comment

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

I like the idea of these internal helper methods that go through and update all of the state whenever something changes. Very clever.

numbered_planets = ["0: List all summaries"]
@planets.each.with_index(1) do |planet, index|
numbered_planet = "#{index}: #{planet.name}"
numbered_planets << numbered_planet

Choose a reason for hiding this comment

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

Seems like you could use the @planet_names array here to simplify this code a bit.

end

def get_distance # might need to convert ',' to '_' for bignums
print "How far is this planet from the Sun in miles? > "

Choose a reason for hiding this comment

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

While I love that you've broken all these out into separate methods, for the questions this might be a little too much functional decomposition. You've got a bunch of small methods here, but they all get called in the same sequence every time. It might be easier to follow if it was all one method.

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