Sam Button Takeaway Challenge #2231
Conversation
There was a problem hiding this comment.
Some really interesting implementation here with quantity. Would love to see some more encapsulation and use of private methods to really clearly define what public interface is that the user can do and what the program does in the background. Think that would help you refine your tests further as well as you don't need to test private methods. And fingers crossed you can figure out your ENV variables as well.
| require './docs/menu' | ||
|
|
||
| describe Menu do | ||
| it 'responds to Menu class' do |
There was a problem hiding this comment.
Don't think this test is needed. Rspec always initialises the subject to be a new instance of the described class
| require './docs/order' | ||
|
|
||
| describe Order do | ||
| let(:menu) { Menu.create } |
There was a problem hiding this comment.
Interesting why .create and not .new ?
| end | ||
|
|
||
| it 'responds to add_to_basket' do | ||
| expect(subject).to respond_to :add_to_basket |
There was a problem hiding this comment.
Think this is being tested below so could probably be refactored out as well
| expect(subject.current_order).to eq(["Guinness"]) | ||
| end | ||
|
|
||
| it 'can add multiple dishes at once' do |
There was a problem hiding this comment.
This is clever I hadn't thought about implementing quantity
| subject(:sms) { described_class.new } | ||
|
|
||
| before do | ||
| allow(sms).to receive(:sms) |
There was a problem hiding this comment.
Much prefer this was of stubbing the SMS method directly rather than trying to stub the client like I did
| quantity.times { @current_order << dish } | ||
| end | ||
|
|
||
| def calculate_price |
There was a problem hiding this comment.
Think some of these methods could be encapsulated into private as the the user doesn't need to calculate the price, the program just needs to do it and the user sees the output
| @sent | ||
| end | ||
|
|
||
| def sms_confirmation |
There was a problem hiding this comment.
Nice idea to have the confirmation message, but again could be in private
|
|
||
| class SMS | ||
|
|
||
| ETA = (Time.now + 3600).strftime("%H:%M") |
There was a problem hiding this comment.
Magic number could be defined elsewhere
Sam Button
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!