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

Ari Herman- Solar System- Octos #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

arrriiii
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 is called automatically when solar-system class/planet class is called on the object ( i.e. planets).
Describe an instance variable you used and what you used it for. All of my planets are instance variables to the class Planet. This allowed me to construct a summary of the planet attributes. --Which I could call later when the user needed info on a particular planet.
Describe what the difference would be if your SolarSystem used an Array vs a Hash. **my SolarSystem class did use an array per request on Wave 2. If it were to use a hash, then I would need to change the iteration in my class to accept key-value pairs. (planets names as the keys and their attributes as the values)
Do you feel like you used consistent formatting throughout your code? yes. ⌘i is my friend.

@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. missing the add_planet method
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 I am quite happy with this submission. Keep up the hard work!

num = 1
@planets.each do |arr|
print "\n#{num}. #{arr.summary} ".cyan
num += 1

Choose a reason for hiding this comment

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

I'm not convinced by the name of the block parameter here. arr is not very descriptive - it's not clear to the reader what type of thing you expect it to be. Calling it something like planet might be a better choice.

@@ -0,0 +1,146 @@
# Solar System project -create an object which contains a

Choose a reason for hiding this comment

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

Ruby files should be named using snake_case. So this one would be solar_system.rb.

The choice is somewhat arbitrary, but when we start working on group projects it will be important for all of us to be doing the same thing.

case info
when "Earth"
puts earth.summary.green
when "Mercury"

Choose a reason for hiding this comment

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

This strategy of using a switch statement makes it difficult to add new planets. Could you do something like adding a method to SolarSystem to look up a planet by name? That would allow you to show info for user planets as well.

end
# Add the planet to the SolarSystem class
planets.push(new_name = Planet.new(new_name, new_color, new_orbit, new_diameter,
new_distance))

Choose a reason for hiding this comment

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

Adding the new planet to planets works, but it's not great form. The purpose of SolarSystem is to keep track of our list of planets, and it's a little confusing to modify the list directly from outside. Instead, you could define a method add_planet on SolarSystem that takes an instance of Planet and adds it to the list.


planets.each do |x|
puts "#{num}. #{x.name}".cyan
num += 1

Choose a reason for hiding this comment

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

This is another place where it might make sense to define a method on SolarSystem rather than accessing the list of planets directly.

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