Skip to content

Instantly share code, notes, and snippets.

Show Gist options
  • Save philsof/2597cb14d3774b524cbf to your computer and use it in GitHub Desktop.
Save philsof/2597cb14d3774b524cbf to your computer and use it in GitHub Desktop.
code-review-pair-kerryimai,plonsker-orange-tree-2-groves-challenge

Good stuff getting to this challenge! Here are my notes on your code:

  • It looks like you have methods on your specific tree classes that are similar. For example, OrangeTree.has_oranges? and PearTree.has_pears?. This is a code smell. You could DRY out your code by putting a has_fruit? method on the FruitTree class. This way, every child class of FruitTree will have a has_fruit? method. Tada! Can you find any other ways to DRY out your code? Hint: look for similarly-worded methods that appear in all of your tree classes.

  • Each of your tree classes has a few lines of code in its age! method that needs to be rethought. Here are those lines, from the AppleTree class:

    if age > 3 && age < 17
      new_number = rand(10..25)
      number = (1..new_number).to_a
      number.map {|apple| @fruit << apple = Apple.new(rand(1..4))}
    end

    I understand what you want to do here (create a random number of Apple objects and encapsulate them into your AppleTree object) but your code as it is written is unnecessarily doing much more than this. Remember: .map takes an existing array and creates & returns a new array. You don't want to do that here, so .map is not a good choice.

    How about something like this:

    if age > 3 && age < 17
      rand(10..25).times do
        @fruit << Apple.new(rand(1..4))
      end
    end

    Notice how the times method gives Ruby the ability to loop a specified number of times, and also how .times can be chanined onto the rand method's return value, to loop a random number of times.

  • Your code is tracking the life status of your trees with a @life instance variable that holds a string, either "alive" or "dead". This is a code smell also. Any attribute that is either "on" or "off" can often be handled better with a boolean. Like this:

    @alive = true

    or the other way:

    @dead = false

    With a boolean value, your dead? method now becomes a lot easier to write:

    def dead?
      @dead # or !@alive if you use @alive
    end

If you have any questions let me know. I hope this helps!

-Phil

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