Branches - Kelsey by kelsk · Pull Request #28 · Ada-C12/solar-system

kelsk · 2019-08-21T05:45:48Z

Solar System

Congratulations! You're submitting your assignment.

Comprehension Questions

Question	Answer
When does the `initialize` method run? What does it do?	"initialize" runs first thing, when the computer encounters the "require_relative" code associated with the "planet" class file.
Why do you imagine we made our instance variables readable but not writable?	We didn't want any users to be able to modify the contents of our solar system once it had been made.
How would your program be different if each planet was stored as a `Hash` instead of an instance of a class?	Each attribute and method of the planet would be accessed using a key/value pair instead of dot syntax.
How would your program be different if your `SolarSystem` class used a `Hash` instead of an `Array` to store the list of planets?	Similar as above, except the Hash would need an attr_writer or attr_accessor to be modified with the list of planets.
There is a software design principle called the SRP. 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?	I could say that my SolarSystem class has one responsibility: Planet management, and that my Planet class has one responsibility: Planet creation. I think SolarSystem might be trying to do a little too much to follow the SRP properly.
How did you organize your `require` statements? Which files needed `require`s, and which did not? What is the pattern?	Only the "main" file used the "require_relative" statement, one each for the SolarSystem class and the Planet class. The require statement needs to be in the file that has code that is creating an instance of that class.

…ely.

…or input their own planets

unused copy

tildeee · 2019-09-03T04:03:35Z

Solar System

What We're Looking For

Feature	Feedback
Baseline
Whitespace (indentation, vertical space, etc)	x
Variable names	x
Git hygiene	x, I prefer commit messages that are less "wave 2 complete" and more "implements adding planet functionality" or "refactors main control flow"
Planet
`initialize` method stores parameters as instance variables with appropriate reader methods	x
`summary` method returns a string	x
SolarSystem
`initialize` creates an empty list of planets	x
`add_planet` takes an instance of `Planet` and adds it to the list	x
`list_planets` returns a string	x
`find_planet_by_name` returns the correct instance of `Planet`	x
CLI
Can list planets and quit	x
Can show planet details	x
Can add a planet	x
Complex functionality is broken out into separate methods	x, it'll be best practice to not nest methods, but keep them next to each other
Overall

Well done on this project, Kelsey! Your object-oriented code is great; it's logical and has good code style. You also did a great job of doing a lot of good code best practices, like making helper methods.

I think that if you take a fresh look at your code in main.rb, you'll find some opportunities to refactor where some of the logic lives so it feels like the user's flow loops in one area (because right now, the code that controls where the user is jumps around main.rb). This would be where I would start if I had more time on this project! Also, I've added some comments to help talk about some code style best practices.

Besides those comments, your code otherwise looks great; you did a great job determining a user flow (with the generic planets, etc). Keep up the good work!

tildeee · 2019-09-03T04:08:02Z

+
+def main
+
+  puts "BIG BANG!"


tildeee · 2019-09-03T04:08:08Z

+  user_response = gets.chomp.upcase
+
+
+  def upload_generic_list_of_planets(system)


Great work making all of these helper methods! They're great and perfect! In the future, we'll probably want these helper methods to be defined outside of the main method (so that they're "siblings" rather than being nested)

tildeee · 2019-09-03T04:08:22Z

+  end
+
+
+  def run_control_loop(system, response)


Hm, this method takes in response, but then immediately re-assigns it in the line after. Does this method actually need to take in a second parameter?

tildeee · 2019-09-03T04:09:13Z

+  def run_control_loop(system, response)
+
+    response = 0
+    while response != 3


Why not when it's 3?

tildeee · 2019-09-03T04:09:24Z

+    @fun_fact = fun_fact
+  end
+
+  attr_reader :name, :color, :mass_kg, :distance_from_sun_km, :fun_fact


Just as a minor point: I think it may be preferable and more easily recognizable of these attr_readers were above the initialize method (and after the class Planet line)

tildeee · 2019-09-03T04:09:35Z

+    if checks_if_zero?
+      return "#{@name} is a #{@color} planet with a mass of #{@mass_kg}kg. It is #{@distance_from_sun_km}km away from the sun. Fun fact about #{@name}: #{@fun_fact}"
+    else
+      raise ArgumentError.new("#{@mass_kg == 0 ? 'Mass (kg)' : 'Distance from sun (km)'} must not equal zero")


Nice ternary!

tildeee · 2019-09-03T04:09:39Z

+    if checks_if_zero?
+      return "#{@name} is a #{@color} planet with a mass of #{@mass_kg}kg. It is #{@distance_from_sun_km}km away from the sun. Fun fact about #{@name}: #{@fun_fact}"
+    else
+      raise ArgumentError.new("#{@mass_kg == 0 ? 'Mass (kg)' : 'Distance from sun (km)'} must not equal zero")


Based on the context of this error and the summary method, this is a completely ideological point, but I wonder if this ArgumentError should be raised or checked somewhere else in the code

tildeee · 2019-09-03T04:09:54Z

+
+    found_planet_summary = found_planet_summary.compact
+
+    # handles multiple planets with the same name


Great work handling this case!

I think that if you had more time, this logic would be a really good place for refactoring

tildeee · 2019-09-03T04:10:05Z

+    end
+
+    if found_planet_summary_string != ""
+      return found_planet_summary_string


Instead of returning a string, it probably would make more sense to return an instance of Planet. That being said, I think if you made that change now, a lot would have to be refactored in your other code!

tildeee · 2019-09-03T04:10:16Z

+  end
+
+  def saturn
+    return "


YAY I LOVE THIS!

tildeee · 2019-09-03T04:10:25Z

+require_relative '../lib/main'
+require_relative '../lib/solar_system'
+
+describe "solar system project" do


Fantastic work making tests! :)

tildeee · 2019-09-03T04:10:31Z

+describe "solar system project" do
+  describe "Planet class instance" do
+
+    it "returns false for mass that equals 0" do


nstead of naming this test as "returns false," we should rename it to "raises an argument error for mass that equals 0"

tildeee · 2019-09-03T04:10:43Z

+      solar_system.planets << Planet.new("earth", 1, 1, 1, 1)
+      found_planet = solar_system.find_planet_by_name("Krypton")
+
+      expect(found_planet).must_match "That planet is not in this solar system."


Just a nice FYI: In the future, we'll see that a lot of libraries in Ruby have a pattern that if there was no match, we get either nil or false or 0 or something like that, rather than an error string.

tildeee · 2019-09-03T04:10:51Z

+
+    end
+
+    it "measures the distance between two planets entered in reverse order" do


Nice tests!! This is an excellent test case to check for thoroughness!

kelsk and others added 14 commits August 19, 2019 15:56

Completed Wave 1

4c6c6be

completed wave 2 steps 1-4

238c0db

added wave 2 step 5

eb383f9

Added minitests for wave 2 step 5

0a212a5

updated code so that the tests pass

839b116

added minitest for wave 2 optional

9ae91ca

added code so that wave 2 optional minitests would pass

87aed4c

Began wave 3 step 1

de46fd5

Removed 'trim_fun_fact' method because it was not behaving appropriat…

b7bef79

…ely.

Completed wave 3 step 2

ccdccd9

Completed wave 3 step 3 and some of 4

3ebd6ea

refactored minor syntax, all code works

e2c48e1

added list of planets to main, ask user if they want to use the list …

dbb1360

…or input their own planets

Delete main.rb

2621b75

unused copy

tildeee reviewed Sep 3, 2019

View reviewed changes

Comment thread lib/main.rb

def main

puts "BIG BANG!"

Copy link
Copy Markdown

tildeee Sep 3, 2019

Choose a reason for hiding this comment

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

YAY!

tildeee reviewed Sep 3, 2019

View reviewed changes

Comment thread lib/main.rb

def run_control_loop(system, response)

response = 0

while response != 3

Copy link
Copy Markdown

tildeee Sep 3, 2019

Choose a reason for hiding this comment

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

Why not when it's 3?

tildeee reviewed Sep 3, 2019

View reviewed changes

Comment thread lib/solar_system.rb

end

def saturn

return "

Copy link
Copy Markdown

tildeee Sep 3, 2019

Choose a reason for hiding this comment

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

YAY I LOVE THIS!

tildeee reviewed Sep 3, 2019

View reviewed changes

		user_response = gets.chomp.upcase


		def upload_generic_list_of_planets(system)


		found_planet_summary = found_planet_summary.compact

		# handles multiple planets with the same name


		end

		it "measures the distance between two planets entered in reverse order" do

Conversation

kelsk commented Aug 21, 2019

Solar System

Comprehension Questions

Uh oh!

tildeee commented Sep 3, 2019

Solar System

What We're Looking For

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tildeee Sep 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tildeee Sep 3, 2019 •

edited

Loading