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

Sockets - Kate N #29

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

Conversation

KateAnnNichols
Copy link

@KateAnnNichols KateAnnNichols commented Feb 27, 2019

Solar System

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Answer
When does the initialize method run? What does it do? The initialize method runs when a new instance of a class is created. This method will pass the argument list through the new instance of the class.
Why do you imagine we made our instance variables readable but not writable? I think we made our instance variables readable but not writable so that user variables do not alter the original classes.
How would your program be different if each planet was stored as a Hash instead of an instance of a class? If each planet was stored in a hash instead of an instance of a hash, more code/steps would have been required to access planet variables.
How would your program be different if your SolarSystem class used a Hash instead of an Array to store the list of planets? If a hash was used instead of an array to store the list of planets, then it would be much more difficult to add, change, and retrieve stored information.
The Single Responsibility Principle (SRP) says that each class should be responsible for exactly one thing. Do your classes follow SRP? What responsibilities do they have? Yes- 1. Solar System is responsible for storing the planets. 2. Planet is responsible for returning/storing each planet's specific information.
How did you organize your require statements? Which files needed requires, and which did not? What is the pattern? I required planet.rb and solar_system.rb in the main.rb file.

@droberts-sea
Copy link

Solar System

What We're Looking For

Feature Feedback
Baseline
Whitespace (indentation, vertical space, etc) yes
Variable names mostly OK - see inline
Git hygiene This needs to improve. Most of your work came in one big commit. You should break this work out into many small commits, each with a descriptive message. If you're having trouble remembering to commit regularly, you might find this article interesting
Planet
initialize method stores parameters as instance variables with appropriate reader methods yes
summary method returns a string yes
SolarSystem
initialize creates an empty list of planets yes
add_planet takes an instance of Planet and adds it to the list yes
list_planets returns a string yes
find_planet_by_name returns the correct instance of Planet yes
CLI
Can list planets and quit yes
Can show planet details yes
Can add a planet yes
Complex functionality is broken out into separate methods yes
Overall

This is a good start. In general your code is well-organized, and it's clear to me that the core learning goal of creating and working with classes has been met.

However, there are a number of small things that definitely need to be fixed around your git habits and the way you're working with files. This is an important part of our curriculum, and will be key on pair projects going forward. If there's still some confusion, please don't hesitate to reach out to one of us, and we'd be happy to help you debug your process.

Other than that, I'm happy with what I see here. Keep up the hard work!


camazotz = Planet.new(“Camazotz”, “red”, 68.9, 22.777, “Ruled by a disembodied brain”)
ixchel = Planet.new(“Ixchel”, “pink”, 49, 10.8E9, “Inhabited by sightless creatures”)
uriel = Planet.new(“Uriel”, “blue”, 10, 411.5E1, “Lots of extremely tall mountains”)

Choose a reason for hiding this comment

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

Your main.rb file had a bunch of weird syntax errors. It somehow ended up with a bunch of angle quotes (“”) instead of straight quotes ("), which very much confuses the Ruby interpreter. You can see here that the syntax highlighting doesn't understand what's going on.

The only way I've seen this happen is via saving code in a Google doc or word doc or similar application, and then copy/pasting it into your editor directly.

This problem was relatively easy to fix, but isn't one I would expect you to be hitting. I have two asks for you here:

  1. Make sure you run your code before submitting it
  2. If this comes up and you're not sure how it happened, come find an instructor

until directions == "exit"
puts "Please enter list planets to see all the planets, planet details to learn more about each planet, or add planet to enter a new planet.
directions = gets.chomp.downcase
if directions == "list planets"EXIT

Choose a reason for hiding this comment

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

More syntax errors: missing and end-quote on line 19, and an extra EXIT at the end of line 21.

def add_new_planet
puts "What is the name of the planet?"
name = gets.chomp.capitalize
puts "What color is the planet?"

Choose a reason for hiding this comment

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

I like that you've broken this functionality out as a separate method, and that you've kept it here in main.rb with the rest of the code to interact with the user. Good organization!


def find_planet_by_name(find_planet)
repeat_name = @planets.find { |planet_name| planet_name.name == find_planet.capitalize }
return repeat_name

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 these variable names. What about something like this:

def find_planet_by_name(name)
  found_planet = @planets.find { |planet| planet.name == name.capitalize }
  return found_planet
end

@droberts-sea
Copy link

I should say that when I cleaned up the problems with the quotes and the extra EXIT, everything worked great.

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