Skip to content

Branches - Xinran#32

Open
gracexinran wants to merge 14 commits intoAda-C12:masterfrom
gracexinran:master
Open

Branches - Xinran#32
gracexinran wants to merge 14 commits intoAda-C12:masterfrom
gracexinran:master

Conversation

@gracexinran
Copy link

Solar System

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Answer
When does the initialize method run? What does it do? When creating an instance of the class; assign attributes for the instance
Why do you imagine we made our instance variables readable but not writable? Avoid re-assigning values to variables, keep instance consistent
How would your program be different if each planet was stored as a Hash instead of an instance of a class? The keys will be the attributes of the class that will take extra steps to create hash for each instance.
How would your program be different if your SolarSystem class used a Hash instead of an Array to store the list of planets? Use names of planets as keys and I can get access to each planet through the names
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? Yes, all the instances have the states and can perform the behaviors of the same class.
How did you organize your require statements? Which files needed requires, and which did not? What is the pattern? require_relative file_name; main.rb requires planet.rb (lib/planet.rb) and solar_system.rb(lib/solar_system.rb), test requires ../main.rb

@tildeee
Copy link

tildeee commented Aug 26, 2019

Solar System

What We're Looking For

Feature Feedback
Baseline
Whitespace (indentation, vertical space, etc) x
Variable names x
Git hygiene x, instead of commit messages that are like "wave 2 complete," I would rather your commit messages describe what kinds of changes in code you made (such as "implements add planet functionality" or "deletes unused solar system methods")
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 theree seems to be missing functionality for showing planet details? :(
Can add a planet x
Complex functionality is broken out into separate methods x
Overall

Fantastic work on this project, Xinran! Your code style is clean, and I can tell that you used OO and composition correctly and well.

In particular, you did a really excellent job of creating helper methods in main.rb. Your code in main looks so good!

You ended up having a bug in your code, though. I've added a comment to detail it.

Also, the project asked for a feature to show planet details, but I don't see a good way for me to do this with your current project submission.

That being said, your code looks great! Also, good job on writing some great unit tests! Good work!

def list_planets
list_planets = "Planets orbiting #{@star_name}"
i = 0
@planets.each do |planet|
Copy link

Choose a reason for hiding this comment

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

Consider refactoring this to use .each_with_index because .each_with_index allows access for an index variable (so you won't need i = 0 or i += 1)


def find_planet_by_name(name)
@planets.each do |planet|
return planet if planet.name.downcase == name.downcase
Copy link

Choose a reason for hiding this comment

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

Nice post-fix conditional syntax :)

end

def find_planet_by_name(name)
@planets.each do |planet|
Copy link

Choose a reason for hiding this comment

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

An idea for refactoring: using the .find method!

while (input.class != Integer && input.class != Float) || input.to_f <= 0
print "Invalid! Input is not a positive value.\nPlease re-input: "
print ">> "
input = gets.chomp
Copy link

@tildeee tildeee Aug 26, 2019

Choose a reason for hiding this comment

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

Your code gets stuck here! To reproduce:

  1. Run main with $ ruby main.rb
  2. Input add planet
  3. For name, input Dee
  4. For color, input red
  5. For mass at kg, input invalid
  6. When asked to re-input, type in 100
  7. Observe that it still says Invalid! Input is not a positive value.

My prediction is that you re-assign input to input = gets.chomp, which will always give back a String, but your while loop relies on input being an Integer or Float.

user_input = gets.chomp
next_step = check_input(user_input, control_panel, control_panel_message)
return next_step
end
Copy link

Choose a reason for hiding this comment

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

Amazing work! Great job making this helper method. It works so nicely!

expect{Planet.new('earth', 'blue-green', 5.972e24, 1.496e8, 222)}.must_raise ArgumentError
end

it "won't raise an error" do
Copy link

Choose a reason for hiding this comment

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

It might be interesting to use this it block to describe a valid Planet that can also read the correct values of name, color, mass_kg, distance_from_sun_km, and fun_fact


# wave-2
describe "SolarSystem" do
before do
Copy link

Choose a reason for hiding this comment

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

Nice use of the before syntax!

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