2
\$\begingroup\$

Is there any way I could make this code more efficient?

def processOrder(quantity, item_list):
    global total
    if quantity > item_list[2]:
        print("There is not enough stock!")
        pass
    else:
        total += item_list[1] * quantity
        item_list[2] -= quantity

total = 0
A = ["Big Mac", float(2.50), 50], ["Large Fries", float(0.50), 200], ["Vegetarian Burger", float(1.00), 20]

print("Welcome to McDonald's")
print("[1]", A[1][0:2],
      "\n[2]", A[1][0:2],
      "\n[2]", A[1][0:2])

while True:
    choice, quantity = (input("\nWhat would you like?\n")).upper(), int(input("\nHow many would you like?\n"))

    if choice == "BIG MAC":
        processOrder(quantity, A[0])
    elif choice == "LARGE FRIES":
        processOrder(quantity, A[1])
    elif choice == "VEGETARIAN BURGER":
        processOrder(quantity, A[2])
    else:
        print("Invalid Item")

    more_items = (input("Do you want to order more items?")).lower()
    if more_items == "yes":
        pass
    else:
        break

print("Thank you for ordering!\nYour total cost is: £" +  str(total))
\$\endgroup\$
  • \$\begingroup\$ I think you have a typo in that you print A[1] three times for the menu. Shouldn't that be each member of A once? \$\endgroup\$ – Snowbody Sep 24 '17 at 2:27
  • \$\begingroup\$ Yes, it was a typo caused by me changing my code to a 2D array (it was originally 3 different lists). Thanks @Snowbody \$\endgroup\$ – Milan Tom Sep 24 '17 at 19:06
  • \$\begingroup\$ You need to make sure your code compiles and works properly before you post it here. Otherwise you risk having your question closed. \$\endgroup\$ – Snowbody Sep 25 '17 at 15:05
5
\$\begingroup\$

PEP 8

Line length

E501:11:80:line too long (107 > 79 characters)
E501:19:80:line too long (110 > 79 characters)

Odd spacing

E222:36:58:multiple spaces after operator

No newline at end

W292:36:71:no newline at end of file

General comments

Main

You want to use __name__ to allow importing without running your code. This explains it better

def main():
    # my code here

if __name__ == "__main__":
    main()

Storing food items

A = ["Big Mac", float(2.50), 50], ["Large Fries", float(0.50), 200], ["Vegetarian Burger", float(1.00), 20]

This may be better represented as either instances of a class, or just namedtuples, making the code more readable, as you access them by name a dictionary may clean up the code for this, rather than a list

\$\endgroup\$
3
\$\begingroup\$

Avoid break and continue if possible

Because they alter the control flow and may complicate the code.

while True:
    # Stuff in the middle...
    more_items = (input("Do you want to order more items?")).lower()
    if more_items == "yes":
        pass
    else:
        break

is better written as

more_items = "yes"
while more_items == "yes":
    # Stuff in the middle...
    more_items = (input("Do you want to order more items?")).lower()

pass does nothing so you (usually) don't need them

To mark a section where more code will be added in the future, a # TODO: comment conveys the intent better.

Avoid code duplication if the duplication is undesirable

The conditionals in

if choice == "BIG MAC":
    processOrder(quantity, A[0])
elif choice == "LARGE FRIES":
    processOrder(quantity, A[1])
elif choice == "VEGETARIAN BURGER":
    processOrder(quantity, A[2])

just repeats the same test in a loop

for i in range(len(A)):
    if choice == A[i][0].upper()
        processOrder(quantity, A[i])

Always verify user input unless failure is an option

choice, quantity = (input("\nWhat would you like?\n")).upper(), int(input("\nHow many would you like?\n"))

can fail if the user inputs a non-numeric answer; also, nothing prevents the user from giving a negative (or floating) number for the quantity, which is likely not what you want.


I've implemented my own version of the program for fun and practice, as shown at the end. This assumes one cashier at one McDonald's. If you want to support more than one instance of this program running, perhaps use a relational database to do the constraint checking instead and handle the race conditions. You can start by looking at the sqlite3 package.

Taking up on jrtapsell's "Storing food items" point, I've crafted a mutable version of namedtuple to better represent variable quantities such as price and stock, because namedtuple, as the name suggests, is a tuple and therefore is immutable. The program also accepts numbered response for the choice so you don't have to type out the name. I used capwords because, well, lazy.

#!/usr/bin/env python

import collections
from string import capwords


def foodplan(price, stock):
    if not str(stock).isdecimal():
        raise ValueError("Can only assign a whole number to stock attribute")
    return type('FoodPlan', (object,), dict(price=float(price), stock=int(stock)))


def get_choice():
    while True:
        option = input("What would you like? ")
        if option.isdecimal() and 0 <= int(option) - 1 < len(choices):
            return list(choices.keys())[int(option) - 1]
        elif capwords(option) in choices.keys():
            return capwords(option)
        else:
            print("Invalid item")


def get_quantity(choice):
    while True:
        quantity = input("How many would you like? ")
        if quantity.isdecimal():
            if int(quantity) <= choices[choice].stock:
                return int(quantity)
            else:
                print("There is not enough stock!")
        else:
            print("Illegal quantity")


choices = collections.OrderedDict({
    "Big Mac": foodplan(2.50, 50),
    "Large Fries": foodplan(0.50, 200),
    "Vegetarian Burger": foodplan(1.00, 20),
})


if __name__ == '__main__':

    orders = dict(zip(choices.keys(), [0] * len(choices)))

    print("Welcome to McDonald's")
    ordering = 'y'
    while ordering == 'y':
        [print("{0}. {1}, £{2}".format(
            i + 1, list(choices.keys())[i], list(choices.values())[i].price
        )) for i in range(len(choices))]
        choice = get_choice()
        quantity = get_quantity(choice)
        orders[choice] += quantity
        choices[choice].stock -= quantity
        ordering = input("Do you want to order more items? [y/*] ").lower()
    print("\nThank you for ordering!\nYour total cost is: £{0}".format(
        sum([orders[choice] * choices[choice].price 
        for choice in choices.keys()])
    ))
\$\endgroup\$
2
\$\begingroup\$

The main issue with your program is its design. It is almost a sequential application, which thing makes it not, or hardly, scalable. Try to take advantage of the other programming paradigms like the functional (Programming with function calls that avoid any global state) and object-oriented ones.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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