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-Bita #48

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

Sockets-Bita #48

wants to merge 3 commits into from

Conversation

Bitaman
Copy link

@Bitaman Bitaman commented Feb 28, 2019

Solar System

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Answer
When does the initialize method run? What does it do? when we create the first instance of a class. it will assign a value to the object that we create
the instance
Why do you imagine we made our instance variables readable but not writable? when we don't want who ever is using the program change the variables, we make them only readable. like the breed of the dog that we created in a dog class. they can change the name, so we make that instance variable writable.
How would your program be different if each planet was stored as a Hash instead of an instance of a class? then for getting the information about the planet we would have to call the key and get the value every time, and that would make it harder if for example we wanted to check the value of two keys in two or more planets. like their distance from each other.
How would your program be different if your SolarSystem class used a Hash instead of an Array to store the list of planets? I guess it would make it a bit harder to find planet by name and also when we print the list of planets for the user it would print the list in different order every time.
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? As the planet class is only responsible for individual planets and the solar system is for planets in one system I think they follow SRP.
How did you organize your require statements? Which files needed requires, and which did not? What is the pattern? the main file needed to require both planets and solar system file. as we interacting with the user in main it should have access to the files that we define our classes and their methods. all the files can require gems if we use them.

Notes to Instructors: I made a couple of mistakes throughout this project. the first one was that I did git init and then added and commited after each wave by the time I realized I was almost done so there is no commit message after every step in my project, then I totally forgot to submit my project on time and didn't do a pull request last night. I made some notes for myself to prevent the same mistakes on the next projects!

@droberts-sea
Copy link

Solar System

What We're Looking For

Feature Feedback
Baseline
Whitespace (indentation, vertical space, etc) mostly good - see inline
Variable names mostly good
Git hygiene I saw your note - I'll be checking for this on the next assignment
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 mostly - see inline
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 no - see inline
Overall

This is a good start, and the code you've written does address the problem. However, there are a large number of small issues with your code, either where it doesn't quite match what we've asked for, or where it's not as clean as it should be. This sort of sloppiness will become more and more of a problem for you as our projects become more complicated and open-ended.

There are two things you can do to help address this:

  • Make sure you understand exactly what we're asking for. Read the project requirements multiple times, slowly. Print them out and take notes. Come up with examples of what your code should look like (method names and parameters, form of user input, etc). Double check with your classmates that you're on the right track.
  • Give your code a final pass before you submit it. Look for variable names, puts statements, big blocks of code that could be broken up, etc. Make sure that you're submitting something you can be proud of.

All that being said, this code does work, and I do believe that the specific learning goals for this assignment were met. Keep working on making your code clean, and keep up the hard work!

if x == 1
list = solar_system.list_planets
puts list
elsif x == 3

Choose a reason for hiding this comment

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

The user should be able to type a command like "list planets" or "quit" for these, not a number.

Also, why are your numbered options out of order? That makes this code just a bit more confusing.

elsif x == 4
puts "What is the name of the name of the planet you want to add?"
name = gets.chomp
puts "What is the color of your planet?"

Choose a reason for hiding this comment

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

The code to view planet details and to add a planet ends up being pretty big. It might be wise to isolate each of these in its own method.

Choose a reason for hiding this comment

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

Also, this is a big long block of code with no breaks. You should split it up using empty lines.


earth = Planet.new("Earth", "blue-green", "5.972e24", "1.496e8", "Only planet known to support life")
puts earth.name
puts earth.fun_fact

Choose a reason for hiding this comment

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

Here you are creating an instance of Planet inside that class. I'm not sure if this will work, or what effect it will have.

In general, please remove all debugging work before submitting your assignment.

def find_planet_by_name(name)
#raise ArgumentError unless @planet.include?(name.capitalize)
@planets.find { |planet| planet.name.downcase == name.downcase }
end

Choose a reason for hiding this comment

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

Good use of an enumerable here.

"#{index + 1}" + "." + item.name
}
puts "Planets orbiting Sun are: "
return rotating_planets

Choose a reason for hiding this comment

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

Instead of putsing here, you should add the output on line 22 to the string you return.

x = 1
while x != 2
puts "What would you like to do?"
puts "1. List planet"

Choose a reason for hiding this comment

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

I don't know that I like the variable name x here - could you call this something more descriptive? Maybe user_choice?


def add_planets(planet)
@planets.push(planet)
end

Choose a reason for hiding this comment

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

Small nitpick: the spec asked for a method named add_planet, not add_planet. This may seem like a small thing, but this sort of error makes everything a little more complex, especially when working with a big team of engineers. Software engineering is all about attention to detail.

Since the method only adds one planet at a time, add_planet is definitely the right choice.

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