Skip to content

Instantly share code, notes, and snippets.

@philsof
Last active January 26, 2016 06:18
Show Gist options
  • Save philsof/64e6e5ae99c4db464527 to your computer and use it in GitHub Desktop.
Save philsof/64e6e5ae99c4db464527 to your computer and use it in GitHub Desktop.
Code review for pair-jesskrich on cookies-and-ovens-challenge

Good effort on this challenge! Here are some comments from me:

###Release 0: Move cookies into and out of the oven###

  • I can move cookies into your oven, but not out of it. Take a look at your ``remove_cookies``` method:

    def remove_cookies
      @contents.each do |cookie|
        if cookie.done == true
          @cooling_rack << cookie
        end
      end
      # contents.delete_if { |cookie| cookie.done == true}
    end

    The above method is iterating through all of the cookies in the contents array, and it is adding the done = true cookies to the cooling_rack. But, this method is not removing the cookies from the contents array (and thus not removing them from the oven).

    I am guessing that you may have tried to remove cookies from @contents while iterating over it with .each, which would not work. Remember: .each is not able to change the array it is iterating over. Read the first answer in this thread if you want to know why .each can't do this.

    For your reference, here is one way to move the cookies from the oven to the cooling rack, utilizing the array methods select and + and -:

    def remove_cookies
      # get all the cookies to remove into an array
      cookies_to_remove = @contents.select { |cookie| cookie.done == true}
      # remove those cookies from the oven
      @contents = @contents - cookies_to_remove
      # add those cookies to the cooling rack
      @cooling_rack = @cooling_rack + cookies_to_remove
    end

Of course, there are other ways to accomplish this. Hopefully this example helps.

###Program Control###

When I was trying to run your code, it was hard for me to follow exactly what was happening. I had to manually use IRB to bake the cookies long enough for them to be ready to remove from the oven. Try to create a controller structure that is easy for others to follow, and that takes the program through all of the release requirements. (It looks like you ended up using your controller.rb file for this, although you still have code in your runner.rb file).

###Missed Requirements### Heads up: It looks like you missed these specific instructions from the challenge, regarding the cookie objects: "Status attribute for cookies, with at least these possible values: :doughy, :almost_ready, :ready, :burned".

###Single Responsiblity### Watch out that you are not mixing responsibilites. On line 54 of your code, this method is both adding cookies to the oven and baking them:

def put_cookies_in_oven(quantity)
 quantity.times {@contents.push(Cinnamon.new).push(ChocolateChip.new)}
 bake!
end

Be sure to keep each method to a single responsibility. One method adds the cookies, another method bakes them. To do both, have your controller call both methods. The methods are like tools: each of them should accomplish one thing. The controller picks up the tools and makes something with them.

Also heads up: it looks like this method adds twice as many cookies to the oven than desired.

Good effort! If you have any questions let me know.

-Phil

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