Jimmy Lyons take-away challenge #2233
Conversation
| @@ -0,0 +1,80 @@ | |||
| ## Takeaway Weekdend Challenge | |||
There was a problem hiding this comment.
Good glaven this is a comprehensive and highly helpful README
| end | ||
|
|
||
| def see_restaurant_menu(restaurant) | ||
| @currently_viewed_menu = Menu.new(restaurant) |
There was a problem hiding this comment.
This is a nice bit of abstraction (think that's the word) - didn't occur to me at all
| end | ||
|
|
||
| def csv_to_hash | ||
| @file.readlines.each do |line| |
There was a problem hiding this comment.
Wish I could comment intelligently on how you've handled this with a .csv file, but it's way beyond my understanding
| @current_order.push({item => @menu.menu_hash[item]}) | ||
| end | ||
|
|
||
| def remove(item) |
There was a problem hiding this comment.
This feels a little complicated for one method,, but I'm sure there are good reasons for that
There was a problem hiding this comment.
Could you use encapsulation to move some of these long operations into a private method and make the main body of the code more readable?
| if current_order.empty? | ||
| @printed_order | ||
| else | ||
| format_order |
| def order_total | ||
| @total = 0 | ||
| calculate_total | ||
| return "£#{'%.2f' % @total}" |
| require 'twilio-ruby' | ||
|
|
||
| class Text | ||
| def initialize(number, acc_sid, a_token) |
There was a problem hiding this comment.
Not sure I understand the thinking here, particularly with the method commented out in order.rb. Would be good to chat it through
| expect(subject.display_restaurants).to be_an_instance_of(String) | ||
| end | ||
|
|
||
| # it 'should list restaurants with corresponding csv files' do |
There was a problem hiding this comment.
I think that best practice is to delete redundant code and comments before pull requesting. (nit-picking)
| expect(order.current_order.length).to eq 1 | ||
| end | ||
|
|
||
| it 'should only add items from the initialised menu' do |
There was a problem hiding this comment.
You could make this slightly more readable. You already have the #add describe block, so probably dont need the 'add' in the it block (nit-picking)
| end | ||
|
|
||
| def see_restaurant_menu(restaurant) | ||
| @currently_viewed_menu = Menu.new(restaurant) |
There was a problem hiding this comment.
Would initializing a new menu and order help here?
| @current_order.push({item => @menu.menu_hash[item]}) | ||
| end | ||
|
|
||
| def remove(item) |
There was a problem hiding this comment.
Could you use encapsulation to move some of these long operations into a private method and make the main body of the code more readable?
There was a problem hiding this comment.
Good implementation of the Takeaway challenge, well done. You've clearly demonstrated the objectives of the week.
There's a good range of unit tests here, but in some cases all you need to do is verify that other methods have been called. https://stackoverflow.com/questions/21262309/rspec-how-to-test-if-a-method-was-called For example, where a method is responsible for calling a private method, or a method of another class.
| @order = nil | ||
| end | ||
|
|
||
| def display_restaurants |
There was a problem hiding this comment.
This feels like an addition to the project specs, which you should be careful about - trying to adhere as closely to the spec as possible is always a priority for projects & tech tests.
| # Can't figure out how to put csv files in a separate folder. | ||
| # It always seems to return an error. | ||
|
|
||
| class Menu |
There was a problem hiding this comment.
In this project, something as simple as a Hash or array of Dish objects is fine to represent the menu.
| array_item = {item => @menu.menu_hash[item]} | ||
| fail "Input error: Item not in current order." unless @current_order.include? array_item | ||
| i = current_order.find_index(array_item) | ||
| @current_order.delete_at(i) |
There was a problem hiding this comment.
Deleting an item from the order is also an additional feature! It's definitely functional, but adds unnecessary complexity to your development and end result.
| # def confirm_order(number, acc_sid, a_token) | ||
| # # method required to send order to restaurant | ||
| # sms = Text.new(number, acc_sid, a_token) | ||
| # end |
There was a problem hiding this comment.
We try to remove commented out code / test snippets before opening PRs for tidiness.
Jimmy Lyons
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