Gawain Hewitt Takeaway challenge #2235
Conversation
| class Takeaway | ||
| attr_reader :menu | ||
| def initialize | ||
| @menu = Menu.new |
There was a problem hiding this comment.
I'm impressed you definitely know what you're doing, especially with the corresponding tests
I'm no expert in dependency injection, but this article near the beginning
https://medium.com/@Bakku1505/introduction-to-dependency-injection-in-ruby-dc238655a278
explains how linking the classes together so "strongly" ie 'def initialize... @menu = Menu.new' can cause issues down the line
| subject.show | ||
| end.to output("1. Chips - £1\n2. Tofu - £2\n3. Broccoli - £1\n4. Ice_cream - £2\n").to_stdout | ||
| end | ||
|
|
There was a problem hiding this comment.
your general ruby syntax and string-morphing is fantastic!
| expect(subject.menu).to receive(:show) | ||
| subject.show_menu | ||
| end | ||
|
|
There was a problem hiding this comment.
again, already thinking about testing injection (looking at commit 1) here is very switched on. nice. took a lot of research/cheating for me to come to tests like this
| @@ -1,15 +1,31 @@ | |||
| require 'takeaway' | |||
| require 'stringio' | |||
There was a problem hiding this comment.
You'll have to clue me in on this
| @summary << order_item | ||
| end | ||
|
|
There was a problem hiding this comment.
Really impressive thought about actual user interface, certainly going beyond what is required. You have my sympathy and respect for keeping your head around it though
| @dishes.each do |dish| | ||
| "food #{dish[:food]}" | ||
| if _dish == dish[:food] | ||
| if the_dish == dish[:food] |
There was a problem hiding this comment.
I feel this could be simplified to a ternary operator
ie 'the_dish == dish[:food] ? true : false
There was a problem hiding this comment.
Maybe this method could also be renamed to something like contains_dish? so it's more explicit on what it does
| end | ||
|
|
||
| def order | ||
| loop do |
There was a problem hiding this comment.
Nice that you've worked with user input as well! I'd suggest looking at extracting further this logic into a new class, maybe named Cli or Interface, which would contain methods such as clarify_order or ask_for_order. It could probably also handle the display of the menu (which is right now in the Menu class). This way you can make the main Takeaway and Menu classes do a bit less, and it also makes it easier to test the input/output, as there will be only one class that directly uses puts or gets, so you remove the need to mock these for the other classes
|
|
||
| context '#show' do | ||
| it 'should display menu' do | ||
| # output = <<~MENU |
There was a problem hiding this comment.
I reckon this didn't work because the heredoc syntax needs the "ending" tag (in your case MENU) to be on its own line, at the very beginning of the line (without any blank space before). This would make it:
it 'should display menu' do
output = <<~MENU
1. Chips - £1
2. Tofu - £2
3. Broccoli - £1
4. Ice_cream - £2
MENU
# ...
end| end | ||
|
|
||
| def confirm_order | ||
| @summary.each do |dish| |
There was a problem hiding this comment.
This could also be part of the new class mentioned above - usually it's good practice to split the code that deals with "presenting" or display, from the code that is dealing with pure data or logic. This way, it's easier to change things related to how looks the program outputs (think formatting, translating, or more complex things!) without touching at the core logic of the program, which stays the same no matter how the output is presented and how the input is provided
| end | ||
|
|
||
| def place_order_with_input(*order) | ||
| input = StringIO.new(order.join("\n") + "\n" + "\n" + "\n") |
| @dishes.each do |dish| | ||
| "food #{dish[:food]}" | ||
| if _dish == dish[:food] | ||
| if the_dish == dish[:food] |
There was a problem hiding this comment.
Maybe this method could also be renamed to something like contains_dish? so it's more explicit on what it does
| @input = input | ||
| @output = output | ||
| @dishes = [{ food: :Chips, price: 1 }, | ||
| { food: :Tofu, price: 2 }, { food: :Broccoli, price: 1 }, |
There was a problem hiding this comment.
Minor one here - it's probably better to leave the dish names as strings rather than symbols, as symbols are mostly used for keys, special configuration values, enumerated values, etc, rather than purely "textual" values
| describe 'Featuretest' do | ||
| context '#order' do | ||
| it 'will ask again if item mispelt' do | ||
| output = place_order_with_input("Brocoli", "Broccoli") |
There was a problem hiding this comment.
Nice one! This test suite should probably contain a bit more tests, at least one testing the whole integration of the different components with a successful order process, and assert the output of the takeaway.order method
Gawain Hewitt
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
My readme is very basic to be honest