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

Queue - Tamiko Terada - solarsystem.rb #49

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

Conversation

TamikoT
Copy link

@TamikoT TamikoT commented Feb 15, 2017

Solar System

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Answer
What was the purpose of the initialize method in your class? In the Planets class, I used the initialize method to assign instance variables based on hash key.
Describe an instance variable you used and what you used it for. I used an instance variable and set it to an empty array that I could push user-defined planet names to.
Describe what the difference would be if your SolarSystem used an Array vs a Hash. If my SolarSystem used a hash I could relate the planetary data to each of the planet names. by having the name of the planet be the key. With an array, I just have a collection of planet names that can be referred to by index.
Do you feel like you used consistent indentation throughout your code? I hope so...

@PilgrimMemoirs
Copy link

Solar System

What We're Looking For

Feature Feedback
Created Custom Class with initialize method & instance variables. Well Done
Used an Array to store a list of planets in the SolarSystem class. Adds_planets should add instances of planet to an array. Right now, a new object gets created every time the learn about method is called, so data would not persist if wanting to change something, like an attribute, of a single planet.
Readable code with consistent indentation. Close, Line 113 has a line indented too much
Created a pull request with your name & a meaningful message. Well Done

Nice addition of pushing yourself to do optional enhancements and continue adding features to this program! I think the addition of the user interface did overcomplicate the structure of the solar system class and did take a way from the requirement of storing planet instances in an array.

My biggest concern is over the add_planets and learn about methods (as stated above). By having the planets created in the add_planets method and using the learn_about method only to return the planet data will refrain from new planet instances having to be created more than needed.

In solar system, @age_in_years should be treated like an attribute. Since you're setting it with a default, it would be better practice to define and set that instance variable in initialize and give it an attr_reader.

LOVE that Ascii art! It is making the instance method look long though. Wrapping it up in a method and calling it in the initialize method will help clean it up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants