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

Solar System - Leanne R #32

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

Conversation

leannerivera
Copy link

Solar System

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Answer
When does the initialize method run? What does it do? It creates a new instance of the class, runs immediately with . new
Why do you imagine we made our instance variables readable but not writable? so user cannot edit any of the information outside of the program
How would your program be different if each planet was stored as a Hash instead of an instance of a class? creating new planets would be pushing a smaller hash into an array. also, keywords will be used to get data instead of methods. methods would have to be defined in the main code.
How would your program be different if your SolarSystem class used a Hash instead of an Array to store the list of planets? would be easier to find planet information, can just search keyword 'name' to see if name is there. easier to pull info when key or value is known
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? one class is for everything to do with the Solar system, while the other is for the planets that go into the solar system. the requirements and methods for both classes are distinct.
How did you organize your require statements? Which files needed requires, and which did not? What is the pattern? when a cross-reference was needed, require is needed, eg for the main.rb which needs info on both planet and solarsystem, a reference to both files are imperative. solar system only requires access to planet because it utilizes information from instances of that class.

@CheezItMan
Copy link

Solar System

What We're Looking For

Feature Feedback
Baseline
Whitespace (indentation, vertical space, etc) Good
Variable names Good
Git hygiene More commits would be good, and avoid using waves as commit messages, instead focus on the functionality you have added.
Planet
initialize method stores parameters as instance variables with appropriate reader methods Check
summary method returns a string Check
SolarSystem
initialize creates an empty list of planets Check
add_planet takes an instance of Planet and adds it to the list Check
list_planets returns a string Check
find_planet_by_name returns the correct instance of Planet Check
CLI
Can list planets and quit Check
Can show planet details Check
Can add a planet Check
Complex functionality is broken out into separate methods Nope
Overall Nice work overall. See my inline comments and let me know any questions you have. You hit all the learning goals. Great work!

Rake::TestTask.new do |t|
t.libs = ["lib"]
t.warning = true
t.test_files = FileList['specs/*_spec.rb']

Choose a reason for hiding this comment

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

Note the Rakefile is looking for a specs folder, but you named it spec (singular).


#This method should create two different instances of Planet and print out some of their attributes.

def main

Choose a reason for hiding this comment

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

This method begs to be broken up into smaller methods.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, thanks I thought everything had to stay into main, that is the only reason why I didn't. Now I know for future.


class SolarSystem
attr_reader :star_name, :planets
attr_writer :planets

Choose a reason for hiding this comment

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

You shouldn't need a writer for planets

Copy link
Author

Choose a reason for hiding this comment

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

Yes, correct, forgot to delete after tests.


# takes the name of a planet as a string, and returns the corresponding instance of Planet. The lookup should be case-insensitive.
def find_planet_by_name(planet_name)
@planets.each do |planet|

Choose a reason for hiding this comment

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

You can also do:

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