Code review for branch pair-silvestrijonathan on data-drill-stack-and-queue-challenge 2/11/16

code-review-for-branch-pair-silvestrijonathan-data-drill-stack-and-queue-challenge.md

Good stuff, here are my notes on your code:

• You are getting this, good work! 👍

• This test does the job. Be aware though: Rspec comes with built-in expect change test capabilities, which can test if an object changes. (For example, if the length of an array increased by 1.) This will come in handy in the very near future.

• Note: You caught this error while I was writing this! Nice work! Here is what I wrote anyway:

  This is a really tricky one to catch and is a good teaching moment: take a look at this test, which currently passes when it is run. This test is supposed to make sure the UnderFlow error in your Queue class fires when you try to shift on an array that doesn't contain anything. But take a look at your test: your test is testing to see if a NameError fires, not an UnderFlow error. Do you know why the test still passes? Because a NameError is firing, because you are calling a method on your queue object that doesn't exist! (Your Queueclass does not have a shift method, it has a shift_first_item method.) Thus, this test is a false positive. Watch out for these!

• The == true at the end of this line is not needed, since is_full? will return a boolean value.

• Be sure to give your variables meaningful names. For example, instead of calling this variable @number perhaps name it something like @max or @max_allowed, this way it is clear exactly what this variable represents.

Keep at it! Good work so far 👍

-Phil