-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Luiza's Takeaway #2226
base: main
Are you sure you want to change the base?
Luiza's Takeaway #2226
Changes from 13 commits
864bf6e
d7ecfc7
1eed378
bcf3030
bd5408a
75dd706
496b167
c8f079b
105ce3f
e4a6ff5
790aacc
c2e7504
04d48e3
f41197c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
/**/.DS_Store | ||
/coverage | ||
|
||
.env | ||
|
||
# Local cache of Rubocop remote config | ||
.rubocop-* |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,4 +10,6 @@ end | |
|
||
group :development, :test do | ||
gem 'rubocop', '1.20' | ||
gem 'twilio-ruby' | ||
gem 'dotenv-rails' | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
require 'twilio-ruby' | ||
require 'dotenv' | ||
Dotenv.load | ||
|
||
class Confirmation | ||
|
||
def initialize | ||
@number = ENV['TO_PHONE_NUMBER'] | ||
end | ||
|
||
def send_text | ||
account_sid = ENV['ACCOUNT_SID'] | ||
auth_token = ENV['AUTH_TOKEN'] | ||
client = Twilio::REST::Client.new(account_sid, auth_token) | ||
|
||
from = ENV['FROM_PHONE_NUMBER'] | ||
|
||
client.messages.create( | ||
from: from, | ||
to: @number, | ||
body: "Thank you! Your order was placed and will be delivered before #{delivery_time}" | ||
) | ||
end | ||
|
||
def delivery_time | ||
time = Time.now | ||
delivered_by = (time + (60 * 60)) | ||
delivered_by.to_s.split()[1][0,5] | ||
end | ||
|
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
|
||
class Menu | ||
|
||
DISHES = [ | ||
{ id: 1, name: 'Spinach & Cheddar Pancakes', price: 7, available?: true, quantity: 0 }, | ||
{ id: 2, name: 'Veggie Breakfast', price: 8, available?: true, quantity: 0 }, | ||
{ id: 3, name: 'Halloumi Sandwhich', price: 7, available?: true, quantity: 0 }, | ||
{ id: 4, name: 'Soup of the Day', price: 8, available?: true, quantity: 0 }, | ||
{ id: 5, name: 'Ceaser Salad', price: 8, available?: true, quantity: 0 }, | ||
{ id: 6, name: 'Pizza', price: 9, available?: true, quantity: 0 }, | ||
{ id: 7, name: 'Vegan Pizza', price: 10, available?: true, quantity: 0 }, | ||
{ id: 8, name: 'Yum Yum Salad', price: 11, available?: true, quantity: 0 }, | ||
{ id: 9, name: 'Singapore Laksa', price: 11, available?: true, quantity: 0 }, | ||
{ id: 10, name: 'Spiced Noodles', price: 11, available?: true, quantity: 0 }, | ||
{ id: 11, name: 'Phad Thai', price: 11, available?: true, quantity: 0 }, | ||
{ id: 12, name: 'Caponata', price: 11, available?: false, quantity: 0 } | ||
].freeze | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are some complexities in using a structure like this, when it comes to traversing each data point later in your implementation to print / change dishes. I'll share an example I wrote for another dev: Hashes are most useful where you have a key value pair - eg. if the only information you needed to record was the item name and price, a simple hash could look like:
For data requiring more than two values, where key/value isn't enough, I would suggest defining a new object, for example:
This way, you could work with an array of Dishes inside your Menu class, eg. @menu = [Dish.new(args), Dish.new(args), etc.] Singular dish data can be accessed with @menu[0].name, etc. Which can be quite a bit easier than traversing a complex hash! |
||
attr_reader :dishes | ||
|
||
def initialize | ||
@dishes = DISHES | ||
end | ||
|
||
def display | ||
@dishes.map do |dish| | ||
p "#{dish[:id]}. #{dish[:name]} - £#{dish[:price]} - #{dish[:available?] ? 'available' : 'unavailable'}" | ||
end | ||
end | ||
|
||
def select_dish(id) | ||
@dishes.select { |dish| dish[:id] == id }.first | ||
end | ||
|
||
def dish_available?(id) | ||
dish = select_dish(id) | ||
dish[:available?] | ||
end | ||
|
||
def id_valid?(id) | ||
@dishes.select { |dish| dish[:id] == id } != [] | ||
end | ||
|
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,93 @@ | ||
require_relative 'menu' | ||
require_relative 'confirmation' | ||
|
||
class Order | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This class is doing a lot of things! Each method is concisely written, but there's something of an array of responsibilities that could potentially be grouped further. If you were to group these methods together following this technique, which further classes would you consider drawing out? https://github.com/makersacademy/skills-workshops/blob/main/practicals/object_oriented_design/encapsulation.md |
||
|
||
attr_reader :current_order, :order_placed | ||
|
||
def initialize | ||
@current_order = [] | ||
@order_placed = false | ||
end | ||
|
||
def show_dishes | ||
@@menu.display | ||
end | ||
|
||
def add_dish(id, quantity) | ||
select_dish(id) | ||
if dish_already_added?(id) | ||
add_by_updating_quantity(id, quantity) | ||
else | ||
@dish[:quantity] = quantity | ||
@current_order << @dish | ||
end | ||
end | ||
|
||
def remove_dish(id) | ||
fail_if_dish_not_added(id) | ||
index = @current_order.index { |dish| dish[:id] == id } | ||
@current_order.delete_at(index) | ||
end | ||
|
||
def update_dish_quantity(id, quantity) | ||
fail_if_dish_not_added(id) | ||
remove_dish(id) if quantity < 1 | ||
@current_order.each { |dish| dish[:quantity] = quantity if dish[:id] == id } | ||
end | ||
|
||
def show_current_order | ||
fail_if_order_empty | ||
@current_order.map do |dish| | ||
p "#{dish[:quantity]} x ##{dish[:id]} #{dish[:name]} - £#{dish[:price] * dish[:quantity]}" | ||
end | ||
puts "Order total: £#{total}" | ||
end | ||
|
||
def total | ||
fail_if_order_empty | ||
@current_order.map { |dish| dish[:price] * dish[:quantity] }.reduce(:+) | ||
end | ||
|
||
def place_order(confirmation) | ||
fail_if_order_empty || (fail 'Order already placed' if @order_placed == true) | ||
@order_placed = true | ||
puts "Your order has been placed. Order summary:" | ||
show_current_order | ||
confirmation.send_text ### this would have had the tel number passed on as argument | ||
end | ||
|
||
private | ||
|
||
@@menu = Menu.new | ||
|
||
def select_dish(id) | ||
fail_if_invalid_id(id) || fail_if_dish_unavailable(id) | ||
@dish = @@menu.select_dish(id) | ||
end | ||
|
||
def add_by_updating_quantity(id, quantity) | ||
@current_order.each { |dish| dish[:quantity] = (dish[:quantity] + quantity) if dish[:id] == id } | ||
end | ||
|
||
def fail_if_invalid_id(id) | ||
fail 'Invalid id' unless @@menu.id_valid?(id) | ||
end | ||
|
||
def fail_if_dish_unavailable(id) | ||
fail 'Dish unavailable' unless @@menu.dish_available?(id) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't necessarily need a new method for each failure being raised. SRP in methods would include barriers to that responsibility being carried out - so I would include the fail message inside the relevant method, and maybe draw out the conditional itself to its own method, rather. |
||
end | ||
|
||
def fail_if_dish_not_added(id) | ||
fail 'Dish has not been added' unless dish_already_added?(id) | ||
end | ||
|
||
def fail_if_order_empty | ||
fail 'Current order empty' if @current_order.empty? | ||
end | ||
|
||
def dish_already_added?(id) | ||
(@current_order.select { |dish| dish[:id] == id }) != [] | ||
end | ||
|
||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class adheres well to SRP, it's succinctly written.
There are some over complexities - like each dish being available or not, but this doesn't affect the rest of the implementation.