-
Notifications
You must be signed in to change notification settings - Fork 44
submitting Solar System? #46
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
base: master
Are you sure you want to change the base?
Conversation
Solar SystemWhat We're Looking For
|
| @@ -0,0 +1,97 @@ | |||
| # Brittany Jones | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should end with .rb
|
|
||
| class Planet | ||
| # Create reader methods to give a user access to read the instance variables. | ||
| attr_accessor :planet_hash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code inside a class should be indented. Either use 1 tab or 2 spaces to indent code.
Also why make this a hash or an accessor?
By being a hash a user of Planet must know the structure of the hash and they keys.
By using attr_accessor you are giving users the ability to change @planet_hash. You probably don't want to do that.
Granted in this project it's a minor issue.
| #Create a SolarSystem class with an @planets instance variable. | ||
| #Make your SolarSystem class take an array of Planets instead of hashes. class will take an array of hashes, instead of hashes. | ||
| class SolarSystem | ||
| attr_accessor :planets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar issue with making this an acessor.
| attr_accessor :planets | ||
| def initialize | ||
| @planets = planets | ||
| @planets = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are blanking out @planets so anything sent into initialize is erased.
Also your initialize method isn't getting sent a parameter.
You should have:
def initialize(planets)
@planets = planets
end|
|
||
| def add_planet(planet) | ||
| @planets.push(planet) | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird indentation here with end
| # Add planets to the @planets array. | ||
| def print | ||
| planets.each do |planet| | ||
| puts planet.name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're doing what's called mixing roles here. Each class should focus on one role. SolarSystem should deal with storing planets and returning a list of planet names. Planet should store planet details and provide getter methods.
The main program's methods can deal with Input and output to the screen. That separation of roles can make things easier to reuse and modify.
| planet_choice = gets.chomp.capitalize | ||
| puts "\n------------------------------------------------------------\n\n" | ||
|
|
||
| case planet_choice |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're not using the SolarSystem instance!
Solar System
Congratulations! You're submitting your assignment.
Comprehension Questions
initializemethod in your class?SolarSystemused anArrayvs aHash.