Conversation
eoinmakers
left a comment
eoinmakers
left a comment
There was a problem hiding this comment.
This is a cleanly written Takeaway challenge, well done.
I left very minor comments inline, but the objectives have been clearly demonstrated here and the user stories have been achieved.
The unit tests are comprehensive and wide ranging.
Well done.
lib/order.rb
Outdated
| end | ||
|
|
||
| def view_menu | ||
| Menu.new |
There was a problem hiding this comment.
You previously grab the items on line 10, so you could re-use that instance of Menu to print the dishes instead.
| "Your total is: £#{@total}. Please use the complete_order function, entering your phone number as an argument, to complete your order" | ||
| end | ||
|
|
||
| def item_added_confirmation |
There was a problem hiding this comment.
This functionality is not mentioned in the spec. You should try to adhere to the spec as closely as possible so you're not introducing unnecessary complexity to your project.
| @total = 0 | ||
| end | ||
|
|
||
| def view_menu |
There was a problem hiding this comment.
Have implemented suggestion to move this to initialize, but have a question:
Which is better? The implemented initialize or the one below?
def initialize(items = Menu.new)
@selection = []
@items = items.items
@total = 0
end
Both seem to get the same results... The first reads more clearly but makes for a longer argument
| @@ -33,6 +33,13 @@ def check_order_prompt | |||
| "Please check your order against your total:" | |||
| end | |||
|
|
|||
There was a problem hiding this comment.
did I understand User Story 3 correctly or did I go overboard?
As a customer
So that I can verify that my order is correct
I would like to check that the total I have been given matches the sum of the various dishes in my order'
did I satisfy this condition simply by giving the customer a 'total' method?
lib/menu.rb
Outdated
| def view_menu | ||
| @items | ||
| end | ||
| #is this method necessary? Taking it out doesn't break tests... |
There was a problem hiding this comment.
is this method necessary? Taking it out doesn't break tests...
spec/sms_spec.rb
Outdated
| expect(subject.sms_sent_confirmation).to eq('A confirmation message has been sent to the number you provided') | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
do these tests actually check an sms has been sent, or just that the class acts as though a message has been sent (e.g. changes sent_confirmation)
| expect(subject).to respond_to(:complete_order).with(1).argument | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
how to actually check that complete_order((ENV['MY_PHONE'])) calls SMS.new.send_sms? I realised I was misusing respond_to above and below comment so changed it above, but now all the test does is check that the method takes one argument. I want to implement a test to make sure that when someone enters their phone number it calls the SMS class...
2 participants
Your name
Please write your full name here to make it easier to find your pull request.
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!