Skip to content
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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Luiza's Takeaway #2226

wants to merge 14 commits into from

Conversation

LGretzk
Copy link

@LGretzk LGretzk commented May 2, 2022

Luiza

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).

  • User story 1: "I would like to see a list of dishes with prices"
  • User story 2: "I would like to be able to select some number of several available dishes"
  • User story 3: "I would like to check that the total I have been given matches the sum of the various dishes in my order"
  • User story 4: "I would like to receive a text such as "Thank you! Your order was placed and will be delivered before 18:52" after I have ordered"

README checklist

Does your README contains instructions for

  • how to install,
  • how to run,
  • and how to test your code?

Here is a pill that can help you write a great README!

Copy link

@eoinmakers eoinmakers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done on achieving the functionality of the user stories.

There is an opportunity to further separate the behaviour of the Order class - I recommended a practical around encapsulation that helps to draw together common behaviours.

Your methods are concisely written, and there's a wide range of unit tests included.

Well done!

@@ -0,0 +1,44 @@

class Menu

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.

require_relative 'menu'
require_relative 'confirmation'

class Order

Choose a reason for hiding this comment

The 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:/makersacademy/skills-workshops/blob/main/practicals/object_oriented_design/encapsulation.md

end

def fail_if_dish_unavailable(id)
fail 'Dish unavailable' unless @@menu.dish_available?(id)

Choose a reason for hiding this comment

The 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.

{ id: 11, name: 'Phad Thai', price: 11, available?: true, quantity: 0 },
{ id: 12, name: 'Caponata', price: 11, available?: false, quantity: 0 }
].freeze

Choose a reason for hiding this comment

The 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:

@menu = { "1. Mattar Paneer" => 12.50, "Black Daal" => 7.50 }

@menu[item_name] will return the cost of that item.

For data requiring more than two values, where key/value isn't enough, I would suggest defining a new object, for example:

class Dish
    def initialize(name, price)
        @name = name
        @price = price
    end

    # getter methods for retrieving information
end

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants