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

Emilce Sanchez - Solar System - Octos #34

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

emilcecarlisa
Copy link

Solar System

Congratulations! You're submitting your assignment.

Comprehension Questions

Question Answer
What was the purpose of the initialize method in your class? It creates a new instance of class SolarSystem and Planet
Describe an instance variable you used and what you used it for. I used the instance variable @planets to create an array of planets.
Describe what the difference would be if your SolarSystem used an Array vs a Hash. In the hash I was able to call the planet using key, value syntax but with an array I used a method that called the instance variables (like (planet) name) in a new object planet.
Do you feel like you used consistent formatting throughout your code? Yes

@emilcecarlisa emilcecarlisa changed the title Emilce Sanchez -- Solar System Emilce Sanchez - Solar System - Octos Feb 12, 2018
@CheezItMan
Copy link

CheezItMan commented Feb 13, 2018

Solar System

What We're Looking For

Feature Feedback
Baseline
Readable code with consistent indentation. Check
Primary Requirements
Created Custom Solar System Class with initialize, add planet & list planets methods, without using puts. You have method doing puts and gets.chomp in your code, see my note in your code. This is a minor problem where you're mixing what the class is doing.
Planet Class Created Check
Created a collection of Planet objects as an instance variable in SolarSystem. Check
Accessor methods created Check, but you made them attr_accessor instead of attr_reader.
Method created to return the Planet's attributes and not use puts Check
Created a user interface to interact with the SolarSystem including adding a planet and viewing a planet's details Check, but your interface is a bit cryptic and not intuitive
Additional Notes Nicely done, you hit all the requirements. I left minor notes in your code. Well done!

# sudo gem install samsouder-titlecase --source=http://gems.github.com

require 'rubygems'
require 'titlecase'

Choose a reason for hiding this comment

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

You're requiring gems, which is new. neat!


class SolarSystem

attr_accessor :planet_details, :choose_planet, :planets, :planet_list, :add_planet

Choose a reason for hiding this comment

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

A couple of issues,

  1. You are making these with attr_accessor which means a user could directly change the instance variables. Instead I would recommend making them attr_reader unless you really want to give a user the ability to reassign the instance variables.
  2. Why are planet_details, choose_planet planet_list and add_planet here? They're methods not instance variables.

return updated_array
end

def add_planet

Choose a reason for hiding this comment

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

This method is part of SolarSystem and it's mixing concerns a bit. It's part of SolarSystem and also dealing with the user and keyboard input and puts. This is a mix of roles. Generally a class should deal with one concept. In this case SolarSystem should focus on being a collection of planets and operations you can do on a SolarSystem. So Solar System does it's thing and methods in the main program can deal with the user.

BTW this is a very minor thing in this project

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