Skip to content

Instantly share code, notes, and snippets.

@philsof
Last active January 26, 2016 06:18
Show Gist options
  • Save philsof/dd5aef55439f04ea1713 to your computer and use it in GitHub Desktop.
Save philsof/dd5aef55439f04ea1713 to your computer and use it in GitHub Desktop.
Code review of Orange Tree 1 challenge for pair-san115,superboyblue branch

Good work! Here are my comments on your code:

Orange Tree class:

age! method: Right now, all of the oranges you create have a diameter of 3. If you want to keep it like this, you could make your code simpler by hard coding the diameter into your Orange class initialize method. Or (much better!), perhaps you could change this method so that each orange is created with a random diameter (like in real life). It would be cool if you did that! (And heads up: the runner.rb file assumes your oranges will have varying diameters. See my note on the runner.rb file below.)

Also heads up: Looks like line 25 will add oranges to a tree even if the tree is already dead (!). Uh oh! How could you rewrite your code to avoid this from happening?

Also you should utilize your attr_writer :dead method here. On line 19, instead of @dead = true you could utilize your attr method by writing this as self.dead=(true) (This samem concept also applies to all of your references to grown_oranges and age. For example, instead of referencing these as instance variables like @grown_oranges you could utilize your attr_reader :grown_oranges method by simply referencing it by writing grown_oranges.)

dead? method: dead? should only be checking the state of @dead; it should not change its state (your age! method is already taking care of that). Remember single responsibility!

has_oranges? method: Heads up: empty? can sometimes return true when you would expect it to return false. Check out this Stack Overflow question page, especially the chart in the second answer, for some nuanced differences between .empty? .nil? .blank? and .present?

Tip: It is better to use .any? in this situation, since it would return 'false' if you array somehow contained only falsey objects.

Also watch your indentation on line 41.

pick_an_orange method:

Good work with .pop, which accomplishes two objectives simultaneously! It both removes an orange and returns it. That is exactly what you need to do here. Perfect!

Orange Tree attr methods: At the beginning of your class definition, it looks like you have some attr methods that you do not utilize in your code: :grown_oranges, :dead, and the entire :attr_accessor line, which is blank. You should either delete the attr methods you are not utilizing, or (much better!) refactor your code so that you are utilizing these attr methods when you read/write state from your instance variables. Hint: for example, on line 19, instead of @dead = true write self.dead=(true)

Also, you should utilize attr_accessor for those attributes that you want to read and write. (For example, instead of attr_reader :age and attr_writer :age you would write attr_accessor :age, which is the same as the reader and writer combined.

The Runner File

You missed a few things in this challenge. Take a look at the runner.rb file. Notice that is calls a variable named basket and a method named avg_diameter. If you run the runner.rb file, you will see that your tree never has more than one orange on it, and the average diameter is blank. To meet these specific requirements of the runner.rb file, you would need to 1) change the name of your grown_oranges variable to basket, and 2) create an avg_diameter method that calculates the average diameter of all the oranges on the tree. (Note: this implies that the diameters of your oranges should not all be the same!) Remember: In these challenges it is always wise to look at the desired end result and code toward that desired result.

I hope that you have time to revisit this challenge to meet these requirements. You can do it!

Tests

Good work on your Orange Tree tests! You wrote them all, and wrote them well. Now you know your code works properly, because you have tests, and your tests pass. Testing is vital to good software development. Good job!

Note though: Your tests on dead? will fail when you refactor dead? to only return the state of @dead. Thus, you will need to refactor those two tests once you refactor dead?

Also, you didn't write any tests for your Orange class (your orange_spec.rb file has no tests in it!) You need to write some tests for your Orange class! You can do it!

Any questions let me know, I am here to help.

-Phil

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment