First pull request #646
First pull request #646
Conversation
|
Changes Unknown when pulling e25234f on rossbenzie:master into ** on makersacademy:master**. |
There was a problem hiding this comment.
Hi Ross!
I like the class structure for the most part — straightforward but still pretty well separated. I think Order might be doing too much though — try writing an "Understands..." comment for it. Think about where you're delegating to Basket too — which bits of the methods should be in Order and which in Basket, and why.
The methods are small and concise, which is great, but sometimes border on low readability: e.g. if I'm using Order and I don't want to worry about reading it, what do Order#add and Order#cancel do? I guess Order#cancel probably cancels the order, but then does Order#add add an order? Turns out it doesn't. We could figure it out but it'd be nicer if we could tell at a glance what the method does.
The use of private methods to make the public methods more readable is super good 👍
Some of the tests do a good job of testing (Basket), some of the others do a less good job. respond_to is good for the first stages of TDD, but if that ends up being your only test on a method then I'd suggest thinking about whether your tests are doing the job they should be. Try breaking a bit of the method and seeing whether your tests fail.
Nice work here!
| "Thank you. Your order was placed and will be delivered before" | ||
| end | ||
| def delivery_time | ||
| "#{(Time.now.hour + 1).modulo(24)}:#{Time.now.min}" |
There was a problem hiding this comment.
Nice — good catch on the modulo thing. This would have been a good opportunity to test #buy to make sure it behaves well at 11pm, midnight, 1am etc — and thereby a good opportunity to use mocking to ensure you didn't have to stay up until midnight just to run your tests.
| end | ||
| def display_basket | ||
| basket.each_item do |item, qty| | ||
| printf("%-20s %5d %8.2f \n", item, qty, menu.price(item)) |
There was a problem hiding this comment.
I've not seen printf used a lot in ruby — but I think this is a neat piece of code. I'd personally spend quite a while trying to figure out how to make it more readable, though. How could I make it clearer what's going on here?
|
Changes Unknown when pulling 6e4ba82 on rossbenzie:master into ** on makersacademy:master**. |
No description provided.