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

Ampers: Abinnet Ainalem (Basic requirements met) #38

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

Conversation

Abiaina
Copy link

@Abiaina Abiaina commented Feb 12, 2018

Solar System

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Answer
What was the purpose of the initialize method in your class? To prime my class with the info necessary for it to define its variables and methods at the specific instance of the class it is set to.
Describe an instance variable you used and what you used it for. I used the instance variable name to store the names of the planets.
Describe what the difference would be if your SolarSystem used an Array vs a Hash. I would have to change one of the methods in the SolarSystem to set the instance variables for name, orbit etc to obtain the info from an array and not a hash. It would work but I would have to change some things in my code
Do you feel like you used consistent formatting throughout your code? No, I just barely put it all together and didn't have time to make sure everything was formatted correctly. I had an issue with how I did an array of hashes I don't think the formatting is right....I looked for examples on stackoverflow and ruby docs and settled on the one I have now.

@Abiaina Abiaina changed the title Basic requirements met Ampers: Abinnet Ainalem (Basic requirements met) Feb 12, 2018
puts "Enter anything else to take a break from learning.\n"
play = gets.chomp.downcase
end
puts "\n Live long and prosper"

Choose a reason for hiding this comment

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

Thanks!

} ]

# UI: welcomes and prompts user for input
puts "Hello and welcome to Planet Gazer"

Choose a reason for hiding this comment

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

Why is this indented?!!

In the future before submitting use Atom's autoindent feature!

puts "\nType the planet name exactly as it appears below to learn more."
puts "Or, enter NEW to create your own planet\n"

solar_system = SolarSystem.new(planets)

Choose a reason for hiding this comment

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

A couple of things here:

  1. You're initializing solar_system with an array of hashes and NOT an array of Planet instances!
  2. You're re-creating the SolarSystem instance each time the loop repeats. That's a bit inefficient. Only put code in a loop that needs to be there.

current_planets = solar_system.cycle_through_system
puts "\n"
current_planets.each {|planet| puts planet.upcase}
user_planet_choice = gets.chomp.downcase

Choose a reason for hiding this comment

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

Why show the planet names in all caps and then downcase the user entry?

# Stores an array of hashes of info on planets
def initialize (planets)
@planets = planets
@ranked_planets = []

Choose a reason for hiding this comment

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

Do you need 2 instance variables? This looks like it should be a local variable holding an array of planet names (and numbers).

@color = planet_info[:color]
end
def user_friendly_display
return "\nAt #@sun_distance million miles from the sun,\nA year on the #@color planet #@name passes in #@year_length days.\n"

Choose a reason for hiding this comment

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

Good


# Cycles through list of planets
# Returns numbered list of planets, w/ respect to sun proximity
def cycle_through_system

Choose a reason for hiding this comment

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

This method should just return a list of planet names, not add elements to another instance variable. If you hadn't recreated the SolarSystem each iteration of the main loop you would end up with double and triple entries for each planet in @ ranked_planets.

@CheezItMan
Copy link

Solar System

What We're Looking For

Feature Feedback
Baseline
Readable code with consistent indentation. As you mentioned, you didn't indent consistently.
Primary Requirements
Created Custom Solar System Class with initialize, add planet & list planets methods, without using puts. Well done here
Planet Class Created Check
Created a collection of Planet objects as an instance variable in SolarSystem. Your SolarSystem instance does NOT use Planet instances.
Accessor methods created Check, although you used attr_accessor for everything and your code doesn't need to give the user write access attr_reader would be better.
Method created to return the Planet's attributes and not use puts Check
Created a user interface to interact with the SolarSystem including adding a planet and viewing a planet's details Check,
Additional Notes You hit most of the requirements, but see my in-line notes The biggest issue is that your SolarSystem doesn't use the Planet class at all.

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