Skip to content

Branches - Eve: Solar System#37

Open
tofuandeve wants to merge 24 commits intoAda-C12:masterfrom
tofuandeve:master
Open

Branches - Eve: Solar System#37
tofuandeve wants to merge 24 commits intoAda-C12:masterfrom
tofuandeve:master

Conversation

@tofuandeve
Copy link

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 run whenever we construct an instance of a class by calling .new. It initialize the instance variables with values being passed in (if needed)
Why do you imagine we made our instance variables readable but not writable? Making instance variables readable so that we can check the values stored in those variables of an instance of a class. Making it not writable to prevent people from changing values of instance variables. An example could be: a class AdaStudent has instance variable named @ssn. Making @ssn readable so that one can use this SSN to fill out Ada's paperwork, but it's important to make this @ssn not writable to prevent Ada from changing its value to "hello this is Ada"
How would your program be different if each planet was stored as a Hash instead of an instance of a class? It would be hard to keep track of the planets when there are too many planets also looking up and modifying a specific planet in these planets would be much harder. And readability might also be a problem
How would your program be different if your SolarSystem class used a Hash instead of an Array to store the list of planets? It depends on how I structure my hash. But the way I iterate through @planets to gather information of planets will be different.
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? Planet class is responsible for constructing and managing information of each Planet instance. SolarSystem is responsible for constructing and managing in formation of each SolarSystem instance
How did you organize your require statements? Which files needed requires, and which did not? What is the pattern? solar_system.rb has Planet instances there for it needs to know about planet.rb ('require' file planet.rb). main.rb uses solar_system.rb which knows about Planet, therefore, only file solar_system.rd is required in main.rb. Test files: solar_system_test.rb test SolarSystem class, file solar_system.rb is required. planet_test.rb test Planet class, file planet.rb is required.

…dated SolarSystem class and main.rb to use new constructor
@tildeee
Copy link

tildeee commented Sep 3, 2019

Solar System

What We're Looking For

Feature Feedback
Baseline
Whitespace (indentation, vertical space, etc) x
Variable names x
Git hygiene x
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
Overall

Eve!!! Your Solar System project is fantastic!!! Your code is thorough and logical. It's clean and covers all of the angles. I'm particularly really thrilled by all of the methods you pulled out into main.rb and how all of the code reused a lot of those helper methods. Also, I'm really excited by your tests-- they're the exact level of detail and coverage I'm looking for.

Your code style is also so nice to read; it's dense, but also so concise and elegant. Your code makes me happy to read!

I have a couple of comments and suggestions for minor areas of improvement.

That being said, your project is great overall. Keep up the good work!


def find_planet_by_name(planet_name)
raise ArgumentError.new("Planet name must be a string") if !(planet_name.instance_of? String)
planets_by_name = @planets.select {|obj| obj.name == planet_name.capitalize}
Copy link

Choose a reason for hiding this comment

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

Nice!

Here's a possible refactoring: using find instead of select. It will give back the element
that matches first. If there is no match, then it will give back nil

def distance_between(planet_name1, planet_name2)
raise ArgumentError.new("Arguments must be strings") if !(planet_name1.instance_of? String) || !(planet_name2.instance_of? String)
first_planet = find_planet_by_name(planet_name1)
second_planet = find_planet_by_name(planet_name2)
Copy link

Choose a reason for hiding this comment

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

Great work re-using the other instance methods!

return input
end

def get_string_input_from_user()
Copy link

Choose a reason for hiding this comment

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

Really thorough and thoughtful logic in here. It anticipates a lot of interesting things users can do. Well done

return Planet.new(name: name, color: color, mass_kg: mass_kg, distance: distance, fact: fact)
end

def get_2_planet_names_from_user(solar_system)
Copy link

Choose a reason for hiding this comment

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

I love how your code looks in this method! It looks so nice!!


Minitest::Reporters.use! Minitest::Reporters::SpecReporter.new

describe "Planet class" do
Copy link

Choose a reason for hiding this comment

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

Really really wonderful tests! They're really thorough and cover all of the important parts. Keep it up!


describe "Planet class" do
describe "Constructor" do
it "Error checks input being passed in for mass_kg" do
Copy link

Choose a reason for hiding this comment

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

Minor nitpick: It may be helpful to include in the test name that this error happens for negative mass_kg (same as the next distance test)

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