9
\$\begingroup\$

Is there a more efficient way to check if what the user has entered (“food_ordered”) is on the “order_menu” without having to use “food_keys” or “food_names”?

order_menu = {("C", "Chips (Scoops)") : 2.80, ("F", "Fish (Battered)") : 2.90, ("Fc", "Fish (Crumbed)") : 4.90, ("Fib", "Filet (Battered)") : 6.90, ("Fic", "Filet (Crumbed)") : 6.90, 
         ("Hd", "Hot Dog") : 2.60, ("S", "Sausage") : 2.60, ("Mp" , "Meat Patty (Homemade)") : 3.90, ("Cr", "Crabstick") : 2.50, ("Sr", "Spring Roll (Homemade)") : 2.80, 
         ("Cr", "Curry Roll (Homemade)"): 2.80, ("Pof", "Potato Fritter (Homemade)") : 1.20, ("Paf", "Paua Fritter (Homemade)") : 5.90, ("Cn", "Chicken Nugget") : 1, 
         ("Mh", "Mini Hot Dog (On a stick)") : 1.20, ("Pf", "Pineapple Fritter") : 2.50}
order_dictionary = {}

food_keys = []
food_names = []
for (food_key, food_name), price in order_menu.items():
    food_keys.append(food_key)
    food_names.append(food_name)

    
def order():
    for food_name, price in order_menu.items():
        print(f"• {food_name[1]:<35} {'('+ food_name[0] + ')': <10} ${price:.2f} each")    
    while True:
        food_ordered = input("Order: (Type 'End' when you have finished) ").title()
        if food_ordered == "End" and len(order_dictionary) > 0:
            break
        elif food_ordered in food_keys or food_ordered in food_names:
            for (key, food_name), price in order_menu.items():
                if food_ordered == key:
                    food_ordered = food_name 
            while True:
                try:
                    quantity_food = int(input("Number of {}: ".format(food_ordered)))
                    break
                except ValueError:
                    print("Please input NUMBERS only.")                
            if quantity_food > 10:
                quantity_food_check = int(input("Are you sure you want x{} {}? (re-enter to confirm): ".format(quantity_food, food_ordered)))
                order_dictionary[food_ordered] = quantity_food
            else:
                order_dictionary[food_ordered] = quantity_food
        else:
            print("Please enter an actual item on the menu!")    

order()
\$\endgroup\$
1
  • 4
    \$\begingroup\$ What do you mean when you say "more efficient"? What is it that you are concerned about? Is it elegance / pythonicness of the code? CPU or memory consumption? \$\endgroup\$
    – sarema
    Commented Aug 8, 2022 at 11:00

4 Answers 4

12
\$\begingroup\$

Your order_menu has not chosen a good key structure. Dictionaries need their keys to be useful for lookup, and you have lookups of two kinds: by abbreviation, and by name. When your lookup happens (based on user input), the most convenient approach is for there to be one dictionary with keys for both abbreviation and name. However, during menu printing, you probably don't want to show a line for both kinds of entry. One option is to have two separate data structures: one for selection, and one for presentation.

Rather than ${price:.2f} consider using locale.currency.

Your user interface is not ideal: you ask for "End" but ignore it if there are no orders. Instead, don't ask for "End" until it's possible to end.

You have a mix of f-strings and .format calls; prefer the former.

You correctly validate-loop quantity_food, but then miss doing this for quantity_food_check.

You need a __main__ guard.

You have some duplicate keys for the menu entries Crabstick and Curry Roll (Homemade).

Currently when a customer makes more than one order of the same menu item, the latest overwrites all others. This is unlikely to respect their intent. Instead I propose that you make a defaultdict and add up all of the quantities that the user requests.

Suggested

from collections import defaultdict
from locale import currency, setlocale, LC_ALL

PRESENTATION_MENU = {
    "c"  : ("Chips (Scoops)", 2.80),
    "f"  : ("Fish (Battered)", 2.90),
    "fc" : ("Fish (Crumbed)", 4.90),
    "fib": ("Filet (Battered)", 6.90),
    "fic": ("Filet (Crumbed)", 6.90),
    "hd" : ("Hot Dog", 2.60),
    "s"  : ("Sausage", 2.60),
    "mp" : ("Meat Patty (Homemade)", 3.90),
    "cr" : ("Crabstick", 2.50),
    "sr" : ("Spring Roll (Homemade)", 2.80),
    "cr" : ("Curry Roll (Homemade)", 2.80),
    "pof": ("Potato Fritter (Homemade)", 1.20),
    "paf": ("Paua Fritter (Homemade)", 5.90),
    "cn" : ("Chicken Nugget", 1.00),
    "mh" : ("Mini Hot Dog (On a stick)", 1.20),
    "pf" : ("Pineapple Fritter", 2.50),
}

ORDER_MENU = PRESENTATION_MENU | {
    name.lower(): (name, price)
    for name, price in PRESENTATION_MENU.values()
}


def print_menu() -> None:
    print('\n'.join(
        f"• {name:<30} ({abbrev.title() + ')': <6} {currency(price)} each"
        for abbrev, (name, price) in PRESENTATION_MENU.items()
    ))
    print()


def get_quantity(name: str) -> int:
    while True:
        try:
            quantity = int(input(f"Number of {name}: "))
        except ValueError:
            print("Please input numbers only.")
            continue
        if quantity < 1:
            print('Please enter a positive number.')
        else:
            return quantity


def order(abbrev: str) -> int:
    food = ORDER_MENU.get(abbrev)
    if food is None:
        print("Please enter an actual item on the menu!")
        return 0

    name, price = food
    quantity = get_quantity(name)

    if quantity > 10:
        print(f"Are you sure you want x{quantity} {name}? (re-enter to confirm).")
        quantity_check = get_quantity(name)
        if quantity_check != quantity:
            return 0
    return quantity


def order_all() -> dict[str, int]:
    orders = defaultdict(int)

    while True:
        abbrev = input('Order: ').lower()
        quantity = order(abbrev)
        if quantity:
            orders[abbrev] = quantity
            break

    while True:
        abbrev = input("Order (or type 'end' when you have finished): ").lower()
        if abbrev == "end":
            break
        quantity = order(abbrev)
        if quantity:
            orders[abbrev] += quantity

    return orders


def main() -> None:
    setlocale(LC_ALL, '')
    print_menu()
    order_all()


if __name__ == '__main__':
    main()
\$\endgroup\$
1
  • \$\begingroup\$ Abbreviation should be named something like "primary key" since that's what its function is \$\endgroup\$
    – qwr
    Commented Aug 10, 2022 at 0:35
3
\$\begingroup\$

Yes, but! Your order_menu is not suited for fast lookup. You can supply additional lookup dictionaries or you can at least ensure not to scan sequentially multiple times.

Currently you do some helper lists, but you still search them sequentially. In

elif food_ordered in food_keys or food_ordered in food_names:

you search sequentially in both lists. In

for (key, food_name), price in order_menu.items():

you again search sequentially for the third time. On large data sets linear search would be a performance problem. Also you save orders as name-quantity dictionary. You will have to lookup the price, thus scanning again.

If your data sets are of limited size and you really want to go without additional indices you can omit the first line and pack the check into the second loop. Also add prices to your order dict.

Lookup structures

Assuming you cannot change the structure of order_menu I'd recommend the preparation of lookup structures like

name_to_price = {n:p for (_, n), p in order_menu.items()}
key_to_name = {k:n for k, n in order_menu}

to support efficient lookup.

Other stuff

  • Your check for quantities > 10 does not work, quantity_food_check is not referenced
  • You do not allow abortion on empty order or in other input stages
  • You do not return the order_dictionary (with or without prices)
\$\endgroup\$
2
\$\begingroup\$

Welcome to code review!

I'm not entirely sure what you are asking for when you write "more efficient". I'll assume you are concerned about CPU performance.

Your code uses a dict as its central data structure, which is appropriate, since you want to map from a type of food to its price. But your choice of using a tuple(str, str) for the key is odd. What you write as a two-way relationship between (food acronym, food name) and price is actually a three-way relationship food acronym -> food name and food acronym -> price.

I assume you wrote it as two-way relationship because that reduces the amount of code needed to solve the problem, but it feels hacky, since you try to hide the complexity of the problem. Therefore your code is less elegant than it could be.

This leads us to the core issue. Since the keys of your dict are tuples, and you want to check if an acronym is the first element of the tuple, you decided to have a list of acronyms and a list of names. That is not efficient.

Collections (which is what we call data types that can collect other data types, like list, set, dict) have different levels of efficiency when it comes to various tasks. You use list, which is normally used when you care about the order of items, which you don't. If you want to check if something is in a list, you need to go through all the items in the list (worst case). That means a list of size n requires n lookups to check for containment.

In contrast, a set does not care for the order of the elements. This is used to make lookups significantly faster. Remember this: a lookup in a set always takes the same amount of time, no matter how large the set is. That sounds unintuitive, but it is correct.

So could you replace your lists by sets?

You could, but there's a better way. Split up your two-way dict into 2 separate dicts. Why? A dict very much behaves like a set internally. That means, it also features the constant lookup-time of sets.

If you want to read more about these concepts, they are called hash table / hash maps (for dicts) or hash sets (for sets).

If apply these recommendations, and clean up the formatting a bit (using black), you get this code.

acronym_price_map = {
    "C": 2.80,
    "F": 2.90,
    "Fc": 4.90,
    "Fib": 6.90,
    "Fic": 6.90,
    "Hd": 2.60,
    "S": 2.60,
    "Mp": 3.90,
    "Cr": 2.50,  # duplicate key with Curry Roll
    "Sr": 2.80,
    # "Cr": 2.80,
    "Pof": 1.20,
    "Paf": 5.90,
    "Cn": 1,
    "Mh": 1.20,
    "Pf": 2.50,
}

acronym_name_map = {
    "C": "Chips (Scoops)",
    "F": "Fish (Battered)",
    "Fc": "Fish (Crumbed)",
    "Fib": "Filet (Battered)",
    "Fic": "Filet (Crumbed)",
    "Hd": "Hot Dog",
    "S": "Sausage",
    "Mp": "Meat Patty (Homemade)",
    "Cr": "Crabstick",
    "Sr": "Spring Roll (Homemade)",
    # "Cr": "Curry Roll (Homemade)",
    "Pof": "Potato Fritter (Homemade)",
    "Paf": "Paua Fritter (Homemade)",
    "Cn": "Chicken Nugget",
    "Mh": "Mini Hot Dog (On a stick)",
    "Pf": "Pineapple Fritter",
}

order_dictionary = {}


def show_menu():
    for acronym, name in acronym_name_map.items():
        price = acronym_price_map[acronym]
        print(f"• {name:<35} {'(' + acronym + ')': <10} ${price:.2f} each")


def order():
    show_menu()
    while True:
        food_ordered = input("Order: (Type 'End' when you have finished) ").title()
        if food_ordered == "End" and len(order_dictionary) > 0:
            break
        elif (
            food_ordered in acronym_name_map.keys()
            or food_ordered in acronym_name_map.values()
        ):
            food_ordered = acronym_name_map.get(food_ordered)
            while True:
                try:
                    quantity_food = int(input("Number of {}: ".format(food_ordered)))
                    break
                except ValueError:
                    print("Please input NUMBERS only.")
            if quantity_food > 10:
                quantity_food_check = int(
                    input(
                        "Are you sure you want x{} {}? (re-enter to confirm): ".format(
                            quantity_food, food_ordered
                        )
                    )
                )
                order_dictionary[food_ordered] = quantity_food
            else:
                order_dictionary[food_ordered] = quantity_food
        else:
            print("Please enter an actual item on the menu!")


order()

There's more that could be improved here, but since you asked about efficiency, let's stop here for now.

\$\endgroup\$
2
\$\begingroup\$

Yes, it is. There are many ways to do it. I suggest using numbers as keys in order_menu nested dict and iterate over it. The user only input the reference number of food name, just like Big Smoke in GTA SA.

Some problems in your code has been noticed in others answers. Here some changes to your code without restructuring it. Conditional handling to some bugs have been added to the code. (See inline comment)

Suggested

# Change order_menu dict to nested dict with numbers as key
order_menu = {1: {"Chips (Scoops)": 2.80},
              2: {"Fish (Battered)": 2.90},
              3: {"Fish (Crumbed)": 4.90},
              4: {"Filet (Battered)": 6.90},
              5: {"Filet (Crumbed)": 6.90},
              6: {"Hot Dog": 2.60},
              7: {"Sausage": 2.60},
              8: {"Meat Patty (Homemade)": 3.90},
              9: {"Crabstick": 2.50},
              10: {"Spring Roll (Homemade)": 2.80},
              11: {"Curry Roll (Homemade)": 2.80},
              12: {"Potato Fritter (Homemade)": 1.20},
              13: {"Paua Fritter (Homemade)": 5.90},
              14: {"Chicken Nugget": 1.00},
              15: {"Mini Hot Dog (On a stick)": 1.20},
              16: {"Pineapple Fritter": 2.50}
              }

order_dictionary = {}


def order():
    for idx, food in order_menu.items():
        for food_names, price in food.items():
            print(f"{idx:<4} {food_names: <27} ${price:.2f} each")  # print value of nested dict
    while True:
        food_ordered = input("Order: (Type 'End' when you have finished) ").title()
        if food_ordered == "End":
            if len(order_dictionary) > 0:  # Added more condition when user hasn't input any number
                break
            else:
                exit()
        elif food_ordered.isdigit() and int(food_ordered) in order_menu:  # check if user input is a number and within order_menu
            num = int(food_ordered)
            for item in order_menu[num]:
                food_ordered = item  # get value of numbers as name
            while True:
                try:
                    quantity_food = int(input("Number of {}: ".format(food_ordered)))
                    if quantity_food < 1:
                        print("Please enter a positive number only")  # Add handling when the input is zero or negative number
                        continue
                    break
                except ValueError:
                    print("Please input NUMBERS only.")
            if quantity_food > 10:
                quantity_food_check = int(input("Are you sure you want x{} of {}? (re-enter to confirm or enter other number): ".format(quantity_food, food_ordered)))
                quantity_food = quantity_food_check  # overwrite quantity_food value
            # Change the structure of order_dictionary with calculated price
            order_dictionary[food_ordered] = {'quantity': quantity_food, 'price': quantity_food * order_menu[num][food_ordered]}  #removed else
        else:
            print("Please enter an actual item on the menu!")


order()
# Added print for order_dictionary
print('\nYour order:')
for i, pair in enumerate(order_dictionary.items()):
    idx, item = pair
    print(f"{i+1:<3} {idx: <27} {int(item['quantity']): <5} ${item['price']:.2f}")

output

1    Chips (Scoops)              $2.80 each
2    Fish (Battered)             $2.90 each
3    Fish (Crumbed)              $4.90 each
4    Filet (Battered)            $6.90 each
5    Filet (Crumbed)             $6.90 each
6    Hot Dog                     $2.60 each
7    Sausage                     $2.60 each
8    Meat Patty (Homemade)       $3.90 each
9    Crabstick                   $2.50 each
10   Spring Roll (Homemade)      $2.80 each
11   Curry Roll (Homemade)       $2.80 each
12   Potato Fritter (Homemade)   $1.20 each
13   Paua Fritter (Homemade)     $5.90 each
14   Chicken Nugget              $1.00 each
15   Mini Hot Dog (On a stick)   $1.20 each
16   Pineapple Fritter           $2.50 each
Order: (Type 'End' when you have finished) What?
Please enter an actual item on the menu!
Order: (Type 'End' when you have finished) 17
Please enter an actual item on the menu!
Order: (Type 'End' when you have finished) 15
Number of Mini Hot Dog (On a stick): 0
Please enter a positive number only
Number of Mini Hot Dog (On a stick): 3
Order: (Type 'End' when you have finished) 6
Number of Hot Dog: -1
Please enter a positive number only
Number of Hot Dog: 2
Order: (Type 'End' when you have finished) 7
Number of Sausage: 13
Are you sure you want x13 of Sausage? (re-enter to confirm or enter other number): 9
Order: (Type 'End' when you have finished) 11
Number of Curry Roll (Homemade): 15
Are you sure you want x15 of Curry Roll (Homemade)? (re-enter to confirm or enter other number): 15
Order: (Type 'End' when you have finished) End

Your order:
1   Mini Hot Dog (On a stick)   3     $3.60
2   Hot Dog                     2     $5.20
3   Sausage                     9     $23.40
4   Curry Roll (Homemade)       15    $42.00

Process finished with exit code 0

I assume you don't have other code (function) to calculate price and items.

\$\endgroup\$
2
  • 3
    \$\begingroup\$ If you are going to number the food items sequentially anyway, there is no need to use a dict, instead you could store the items (just the full name and price) in a list like food_menu = [{'Chips', 2.80}, {'Fish', 4,90}, ...] , and then do something like if int(food_ordered) < len(food_menu): name, price = food_menu(int(food_ordered)). You might want to add/subtract 1 as appropriate to let the numbering start at 1 instead of 0. \$\endgroup\$
    – G. Sliepen
    Commented Aug 9, 2022 at 10:20
  • \$\begingroup\$ @G.Sliepen Totally agree! I prefer a list or multiple list to stores values and when doing sequential number by index. But the author's code refer to dict for references and for storing output result in the first place. Anyway, I'll keep that in mind. \$\endgroup\$
    – Arifa Chan
    Commented Aug 9, 2022 at 10:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.