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

Branches - Vi Reigosa #42

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

Conversation

criacuervos
Copy link

Solar System

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Answer
When does the initialize method run? What does it do? This method runs in our files where we define a class. It is a constructor which instantiates the setup of arguments passed into it.
Why do you imagine we made our instance variables readable but not writable? We would only need to make them writable if we were reassigning our variables. Instead, for this program, they are firmly only to be read.
How would your program be different if each planet was stored as a Hash instead of an instance of a class? If it were a hash we wouldn't be able to define state and behavior to each instance of a planet. Instead they would exist as key value pairs that would require iteration.
How would your program be different if your SolarSystem class used a Hash instead of an Array to store the list of planets? A hash would be more ideal, since each planet has a "one to many" relationship with various pieces of information unique to that planet. In an array, there would definitely have to be nested arrays of string elements which belong to the array of each planet.
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 believe so. My classes 'Planet' and 'SolarSystem' are responsible for creating instances of each of their namesakes.
How did you organize your require statements? Which files needed requires, and which did not? What is the pattern? I put 'require_relative' for both the Planet and SolarSystem classes in the main.rb file, where their classes/methods are being passed in

@tildeee
Copy link

tildeee commented Aug 30, 2019

Solar System

What We're Looking For

Feature Feedback
Baseline
Whitespace (indentation, vertical space, etc) x
Variable names x
Git hygiene x, I encourage you to keep up small commits with descriptive commit messages!
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 there is opportunity for refactoring here!
Overall

Vi! Well done on this project! You did a great job on this project. In particular, your practice on object-oriented programming and creating classes with the right instance variables and instance methods is great! It looks exactly how I would want it to look

There's opportunity to see what parts of the main.rb file could be refactored into using smaller methods if you had more time on this project. Also, using git in future projects more regularly could be a nice thing to grow in during the upcoming weeks

That being said, well done overall; keep up working with and getting comfortable with classes, instances, instance variables within a class, and instance methods!

require_relative 'solar_system'

def main
earth = Planet.new('Earth', 'Blue-green', 5.972e24, 1.496e8, 'Only planet known to support capitalism :/')
Copy link

Choose a reason for hiding this comment

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

rip


def main
earth = Planet.new('Earth', 'Blue-green', 5.972e24, 1.496e8, 'Only planet known to support capitalism :/')
venus = Planet.new('Venus', 'Pink', 8.6e24, 2.36e8, 'Gayest planet probably!')
Copy link

Choose a reason for hiding this comment

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

:)

puts found_planet.summary
else
puts "thats not a planet name, sorry buddy try again!"
next
Copy link

Choose a reason for hiding this comment

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

Nice! Clever way to make this work


if vega.find_planet_by_name(new_planet_name) != nil
puts "that planet already exists >:( try another"
next
Copy link

Choose a reason for hiding this comment

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

Nice!

end

def list_planets
allplanets = ""
Copy link

Choose a reason for hiding this comment

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

Minor nitpick: Ruby naming standards would probably format this as all_planets


return planets.find { |planet|
planet.name.downcase == planet_name.downcase
}
Copy link

Choose a reason for hiding this comment

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

Well done! Using .find means that if the planet wasn't found, it will return nil, which gets used appropriately in main.


return planets.find { |planet|
planet.name.downcase == planet_name.downcase
}
Copy link

Choose a reason for hiding this comment

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

Just for the thoroughness of seeing many kinds of syntax (I have no preference), I want to share that this syntax is the same:

    return planets.find do |planet|
      planet.name.downcase == planet_name.downcase
    end

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