8
\$\begingroup\$

I have made a phone order pizza ordering program for my Digital Tech assignment. I would appreciate feedback on the code quality (increasing efficiency, whilst maintaining readability). The full code is runnable on repl.it, and is also pasted below.

This is a repost with an updated version of my code for review. Pizza Ordering Program v1 is here. Compared to v1, I have capitalized the constants, and moved the logic to inside the Order class, simplifying the actual run loop.

The full, working code is below:

"""Onehunga Pizzas phone orders"""

import re
import sys

# CONFIG #
# maximum number of pizzas in one order
MAX_PIZZAS = 5
# delivery charge (in $)
DELIVERY_CHARGE = 3.00
# list of dictionaries (pizzas) with name and price
PIZZAS_AVAILABLE = (
    {"name": "Hawaiian",             "price": 8.5},
    {"name": "Meat Lovers",          "price": 8.5},
    {"name": "Pepperoni",            "price": 8.5},
    {"name": "Ham & Cheese",         "price": 8.5},
    {"name": "Classic Cheese",       "price": 8.5},
    {"name": "Veg Hot 'n' Spicy",    "price": 8.5},
    {"name": "Beef & Onion",         "price": 8.5},
    {"name": "Seafood Deluxe",       "price": 13.5},
    {"name": "Summer Shrimp",        "price": 13.5},
    {"name": "BBQ Bacon & Mushroom", "price": 13.5},
    {"name": "BBQ Hawaiian",         "price": 13.5},
    {"name": "Italiano",             "price": 13.5},
)
# END CONFIG #


def get_input(regex, input_message=None, error_message=None):
    """Gets valid input, validated using regular expressions."""
    # loops until input is valid ("break" is called)
    while True:
        # input to validate, input prompt is as specified
        if input_message:
            user_input = input(str(input_message))
        else:
            user_input = input()
        user_input = user_input.lower().strip()
        # check if the user wants to quit or cancel the order
        if user_input == "qq" or user_input == "quit":
            sys.exit()
        elif user_input == "cc" or user_input == "cancel":
            return "CANCEL"

        # check if the input matches the regex provided
        if re.match(regex, user_input, re.IGNORECASE):
            break

        # if it doesn't match, and an error message has been specified
        if error_message:
            print(str(error_message))

    return user_input


def print_line(line):
    """ Prints "| <message> |". Used for print_order()."""
    print("| {:54} |".format(line))


def print_order(order):
    """Prints the order details to the screen"""
    print_line("Name: " + order.name)
    print_line("Order type: " + ("Pickup" if order.pickup else "Delivery"))
    if not order.pickup:
        print_line("Delivery address: " + order.address)
        print_line("Customer phone number: " + order.phone)
    print_line("")
    print_line("Order summary:{:15}Price each:{:5}Subtotal:".format("", ""))
    for pizza in order.pizzas:
        print_line("{:5}x {:22}{:5}${:5.2f}{:8}${:>5.2f}".format(
            pizza["amount"], pizza["name"], "",
            pizza["price"], "", pizza["price"]*pizza["amount"]))
    if not order.pickup:
        print_line("{:4}Delivery charge{:29}${:>5.2f}".format(
            "", "", DELIVERY_CHARGE))

    print_line("{:48}------".format(""))
    print_line("{:40} Total: ${:.2f}".format("", order.cost))


class Order():
    """Holds each order's information, can get the information itself."""
    def __init__(self):
        self.name = ""
        self.pickup = False
        self.address = None
        self.phone = None
        self.pizzas = []
        self.cost = 0

    def get_pickup(self):
        user_input = get_input(
            r"$|(?:P|D)",
            "Pickup or delivery? [Pickup]:",
            "Please enter a 'p' (pickup) or a 'd' (delivery)")
        if user_input == "CANCEL":
            return "CANCEL"
        self.pickup = user_input.lower().startswith("p") or not user_input

    def get_name(self):
        user_input = get_input(
            r"[A-Z]+$",
            "Enter customer name:",
            "Name must only contain letters")
        if user_input == "CANCEL":
            return "CANCEL"
        self.name = user_input[:48]

    def get_address(self):
        user_input = get_input(
            r"[ -/\w]+$",
            "Delivery address:",
            "Address must only contain alphanumeric characters")
        if user_input == "CANCEL":
            return "CANCEL"
        self.address = user_input[:36]

    def get_phone(self):
        user_input = get_input(
            r"\d+$",
            "Phone number:",
            "Phone number must only contain numbers")
        if user_input == "CANCEL":
            return "CANCEL"
        self.phone = user_input[:11]

    def get_pizzas(self):
        # loop to get valid input, needs to be more specific than
        # just regex (needs to be greater than 0, less than 5)
        while True:
            user_input = get_input(
                r"\d$",
                "Number of pizzas to order:",
                "Must be a digit, 5 or less")
            if user_input == "CANCEL":
                return "CANCEL"

            if 0 < int(user_input) <= MAX_PIZZAS:
                number_pizzas = int(user_input)
                break
            else:
                print("Must be a digit, 5 or less (but more than 0)")

        print("\nWhat pizzas would you like to order?")
        for i, pizza in enumerate(PIZZAS_AVAILABLE):
            # each pizza's number is its index (i) + 1,
            # so the first pizza is 1
            print("{}: {}".format(str(i+1).zfill(2), pizza["name"]))

        print("\nEnter your selection number for each pizza you want to buy")
        for i in range(number_pizzas):
            # loop to get valid input
            while True:
                user_input = get_input(
                    r"\d\d?$",
                    "Pizza #{} of {}:".format(i+1, number_pizzas),
                    "Pizza selection number must "
                    "correspond to those listed above")
                if user_input == "CANCEL":
                    return "CANCEL"

                try:
                    # 0 is not a valid pizza number
                    if int(user_input) == 0:
                        raise IndexError
                    # selects the pizza based on user_input
                    # gives IndexError if not valid (e.g. 23 is not valid)
                    to_add = PIZZAS_AVAILABLE[int(user_input)-1]

                except IndexError:
                    print("Pizza selection number must "
                          "correspond to those listed above")

                # if there has been no error,
                else:
                    # input is valid, break from the while loop
                    break

            # if the pizza has already been ordered,
            # increment the amount ordered
            for ordered in self.pizzas:
                if to_add["name"] == ordered["name"]:
                    ordered["amount"] += 1
                    break
            # else add the pizza to the order list
            else:
                to_add["amount"] = 1
                order.pizzas.append(to_add)

    def get_cost(self):
        cost = sum(
            pizza["price"]*pizza["amount"]
            for pizza in self.pizzas)
        if not self.pickup:
            cost += DELIVERY_CHARGE
        self.cost = cost

    def get_details(self):
        if self.get_pickup() == "CANCEL":
            return "CANCEL"
        if self.get_name() == "CANCEL":
            return "CANCEL"
        if not self.pickup:
            if self.get_address() == "CANCEL":
                return "CANCEL"
            if self.get_phone() == "CANCEL":
                return "CANCEL"
        if self.get_pizzas() == "CANCEL":
            return "CANCEL"
        if self.get_cost() == "CANCEL":
            return "CANCEL"


# if __name__ == "__main__":
if __name__ == "builtins":  # for repl.it
    print(
        "== Onehunga Pizzas ==\n"
        "==  Order Manager  ==\n"
        "Enter 'CC' to cancel order, or 'QQ' to exit program at any time.\n"
        "The first letter of a word is usually only required as input.\n"
        "A word [enclosed] in brackets is the default option.\n")

    # list to hold all completed orders
    orders = []

    # sorts pizza list by price, then alphabetically
    PIZZAS_AVAILABLE = sorted(
        PIZZAS_AVAILABLE,
        key=lambda k: (k["price"], k["name"]))

    # loop to get orders until told to quit (quits through sys.exit)
    while True:
        order = Order()
        if not order.get_details() == "CANCEL":
            print("\nOrder saved. Order was:")
            print_order(order)
            orders.append(order)
        else:
            print("\nOrder cancelled.")

        user_input = get_input(
            r"$|(?:Y|N|O).*",
            "Would you like to enter another order or view all "
            "previous orders? [Yes]/No/Orders:",
            "Only yes/no or \"orders\" responses allowed")
        if user_input.lower().startswith("n"):
            sys.exit()
        elif user_input.lower().startswith("o"):
            # print orders
            for i, order in enumerate(orders):
                # make nice box around orders, special title at top
                if i == 0:
                    print("-"*23 + " ALL ORDERS " + "-"*23)
                else:
                    print("|" + "-"*56 + "|")
                print_order(order)
                # if it is the last order, close the box
                if i == len(orders) - 1:
                    print("-" * 58)
\$\endgroup\$
2
\$\begingroup\$

Here are some observations and suggestions:

  • I think you can either define the PIZZAS_AVAILABLE in the correct expected order or sort right after defining it instead of sorting it inside the main execution block
  • you can remove redundant () after the class name:

    class Order:
    
  • you can use the inline if/else for the user_input variable:

    user_input = input(str(input_message) if input_message else '') 
    

    or, even better - define the default input_message value as an empty string. Then, you would be able to do:

    user_input = input(str(input_message)).lower().strip()
    
  • instead of defining a separate print_order() function, consider defining the __str__() magic method on the Order class. Then, you'll be able to do:

    print(order)
    

    where order is an instance of the Order class

  • I don't particularly like the overall design of the Order class - it is currently tied to the user inputs that come from the multiple get_input() calls which makes the user interaction logic scattered across the whole program. Not sure how big of an issue this is, but I would expect the Order class to be agnostic of where the order data is coming from (referring to Single responsibility principle)
  • the get_details() is probably not the best name for a method that is checking if an order should be canceled or not
  • consider defining pickup, name and other things you get via get_*() methods as properties
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks, for the reply, I'd completely forgotten about__str__. Would you recommend moving the input logic out of the class and into separate functions? get_details is supposed to run all the get_* functions and get all the order's details. I have to be able to cancel the order at any time, the cancel will be entered as input, and somehow needs to get back to my get orders loop. That's what all the if x == "CANCEL" are for. If I was to define the get_* as properties, wouldn't the Order class still have the input logic in it? If input is invalid, it immediately needs retry until valid. \$\endgroup\$ – DarkMatterMatt Apr 5 '17 at 19:48
  • \$\begingroup\$ repl.it/GjNe/92 Updated to fit points 1, 2, 3 &4 \$\endgroup\$ – DarkMatterMatt Apr 5 '17 at 20:15
  • \$\begingroup\$ @DarkMatterMatt yes, it would be great to move the input validation logic from outside of the Order class. About get_details() - yeah, you are right. In this case, I'd still improve that "if canceled" check, since it is not immediately obvious that aside from checking if an order is canceled, you also grab the order information..I would probably post a separate follow-up question for the second round of reviews, or wait if there will be other answers with other points..thanks! \$\endgroup\$ – alecxe Apr 5 '17 at 21:08
0
\$\begingroup\$

This is going to be a small one, but I believe it would make more sense for PIZZAS_AVAILABLE to be a dict and not a tuple:

PIZZAS_AVAILABLE = {
    "Hawaiian": 8.5,
    "Meat Lovers": 8.5,
    "Pepperoni": 8.5
    ...
}
| improve this answer | |
\$\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.