Luke's takeaway challenge pull request - #2224
Conversation
lukestorey95
changed the title
SamButton12
left a comment
SamButton12
left a comment
There was a problem hiding this comment.
Hi Luke, very nicely organised and readable code throughout. I've tried to pose some questions, but I suggest reading through the review with your own code in mind as well as having Eoin take a look. Great effort this weekend though!
| 4. Write the minimum code to make test pass | ||
| 5. Refactor and ensure tests still pass | ||
| 6. Repeat step 2 and ensure behaviour works as intended | ||
|
|
There was a problem hiding this comment.
I really like your readme. It sets a tone for the rest of the code.
| @@ -0,0 +1,11 @@ | |||
| describe 'integration test' do | |||
| xit 'should not raise error' do | |||
There was a problem hiding this comment.
Could you have written about future features/testing and any unused tests in the readme?
There was a problem hiding this comment.
For an integration or feature test, you could execute the methods to replicate a full user journey - placing an order and confirming the order total.
| subject(:order) { Order.new } | ||
| dishes = [{ pizza: 9.50 }, { pasta: 8.20 }] | ||
|
|
||
| describe '#add_to_basket' do |
There was a problem hiding this comment.
Really easy to read Rspec describe and context blocks, well done
| end | ||
|
|
||
| def order_complete? | ||
| fail "Order already completed!" if complete_status == true |
There was a problem hiding this comment.
What would be the differences of using raise in this situation?
| @@ -0,0 +1,46 @@ | |||
| class Takeaway | |||
There was a problem hiding this comment.
Very easy to read, all method operations encapsulated into private methods.
eoinmakers
left a comment
eoinmakers
left a comment
There was a problem hiding this comment.
This is a great implementation of the Takeaway challenge - you've clearly demonstrated the learning objectives of the week. Nice work!
There are places in which the implementation is maybe on the more complex side - ie. the use of a CSV file and whether or not a dish is available, but it's not more complex by a large margin, and I think it can be nice to experiment with things like CSVs. in weekend challenges. In tech tests, close adherence to the exact spec is a priority.
You demonstrate a wide array of unit tests particularly in your Takeaway spec - and I left a comment around the integration test currently included.
Nice work!
| end | ||
| end | ||
|
|
||
| def true?(available) |
There was a problem hiding this comment.
I'd recommend adding more context to this method name, eg. is_available? rather than just true?
|
|
||
| def filter_dishes_by_available | ||
| self.available_dishes = dishes.select do |dish| | ||
| dish[:available] == true |
There was a problem hiding this comment.
The functionality around whether a dish is available or not is adding a slight degree of complexity that isn't asked for in the spec - do try to keep your implementation as simple as possible by following the spec closely. I think the 'available'ness mentioned in the user story simply refers to dishes that are on sale at the restaurant.
| @@ -0,0 +1,11 @@ | |||
| describe 'integration test' do | |||
| xit 'should not raise error' do | |||
There was a problem hiding this comment.
For an integration or feature test, you could execute the methods to replicate a full user journey - placing an order and confirming the order total.
3 participants
Your name
Luke Storey
User stories
Please list which user stories you've implemented (delete the ones that don't apply).
README checklist
Does your README contains instructions for
Here is a pill that can help you write a great README!