25
\$\begingroup\$

Today I learnt the basics of OOP. I have tried to apply them to this coffee machine project. However I'm still a beginner, and so I feel my code can be improved. Are there any tips, trick or other other advice I can follow to improve the look, runtime, readability or take full advantage of OOP?

This code is simulating a coffee machine which asks for 4 different actions; buy, fill, remaining, and exit.

When you enter buy, the program asks you which type of coffee you want. Here you can enter 1, 2, 3 or back - if you change your mind about getting coffee. Each coffee has different requirements for the supplies it needs to make a coffee. If there are not enough supplies available, in the coffee machine, then no coffee is made and a prompt appears. If there are enough supplies, the requirements for your selected coffee get deducted from the supplies available and a prompt showing it was successful appears.

The function fill allows you to add to the supplies in the CoffeeMachine class.

Remaining display the current quantities of supplies for each of the materials in the coffee machine. Such as water, milk, coffee beans, cups, and money.

Exit allows the user to stop the program.

#First OOP Project
class CoffeeMachine:

    running = False

    def __init__(self, water, milk, coffee_beans, cups, money):
        # quantities of items the coffee machine already had
        self.water = water
        self.milk = milk
        self.coffee_beans = coffee_beans
        self.cups = cups
        self.money = money

        #if the machine isnt running then start running
        if not CoffeeMachine.running:
            self.start()

    def start(self):
        self.running = True
        self.action = input("Write action (buy, fill, take, remaining, exit):\n")
        print()
        #possible choices to perform in the coffee machine
        if self.action == "buy":
            self.buy()
        elif self.action == "fill":
            self.fill()
        elif self.action == "take":
            self.take()
        elif self.action == "exit":
            exit()
        elif self.action == "remaining":
            self.status()

    def return_to_menu(self): # returns to the menu after an action
        print()
        self.start()

    def available_check(self): # checks if it can afford making that type of coffee at the moment
        self.not_available = "" # by checking whether the supplies goes below 0 after it is deducted
        if self.water - self.reduced[0] < 0:
            self.not_available = "water"
        elif self.milk - self.reduced[1] < 0:
            self.not_available = "milk"
        elif self.coffee_beans - self.reduced[2] < 0:
            self.not_available = "coffee beans"
        elif self.cups - self.reduced[3] < 0:
            self.not_available = "disposable cups"

        if self.not_available != "": # if something was detected to be below zero after deduction
            print(f"Sorry, not enough {self.not_available}!")
            return False
        else: # if everything is enough to make the coffee
            print("I have enough resources, making you a coffee!")
            return True

    def deduct_supplies(self): # performs operation from the reduced list, based on the coffee chosen
        self.water -= self.reduced[0]
        self.milk -= self.reduced[1]
        self.coffee_beans -= self.reduced[2]
        self.cups -= self.reduced[3]
        self.money += self.reduced[4]

    def buy(self):
        self.choice = input("What do you want to buy? 1 - espresso, 2 - latte, 3 - cappuccino, back - to main menu:\n")
        if self.choice == '1':
            self.reduced = [250, 0, 16, 1, 4] # water, milk, coffee beans, cups, money
            if self.available_check(): # checks if supplies are available
                self.deduct_supplies() # if it is, then it deducts

        elif self.choice == '2':
            self.reduced = [350, 75, 20, 1, 7]
            if self.available_check():
                self.deduct_supplies()

        elif self.choice == "3":
            self.reduced = [200, 100, 12, 1, 6]
            if self.available_check():
                self.deduct_supplies()

        elif self.choice == "back": # if the user changed his mind
            self.return_to_menu()

        self.return_to_menu()

    def fill(self): # for adding supplies to the machine
        self.water += int(input("Write how many ml of water do you want to add:\n"))
        self.milk += int(input("Write how many ml of milk do you want to add:\n"))
        self.coffee_beans += int(input("Write how many grams of coffee beans do you want to add:\n"))
        self.cups += int(input("Write how many disposable cups of coffee do you want to add:\n"))
        self.return_to_menu()

    def take(self): # for taking the money from the machine
        print(f"I gave you ${self.money}")
        self.money -= self.money
        self.return_to_menu()

    def status(self): # to display the quantities of supplies in the machine at the moment
        print(f"The coffee machine has:")
        print(f"{self.water} of water")
        print(f"{self.milk} of milk")
        print(f"{self.coffee_beans} of coffee beans")
        print(f"{self.cups} of disposable cups")
        print(f"${self.money} of money")
        self.return_to_menu()

CoffeeMachine(400, 540, 120, 9, 550) # specify the quantities of supplies at the beginning
            # water, milk, coffee beans, disposable cups, money

\$\endgroup\$
4
  • \$\begingroup\$ alright i did add comments to the program to make it easier \$\endgroup\$
    – DeltaHaxor
    Apr 12, 2020 at 19:51
  • 1
    \$\begingroup\$ Thank you for the added description. I hope you get a good answer now :D \$\endgroup\$
    – Peilonrayz
    Apr 12, 2020 at 20:16
  • \$\begingroup\$ FYI what you are building here is called a state machine. That is, the machine has a particular internal state, it takes in a sequence of actions as inputs, each action might or might not cause a state change, and might or might not cause an output that is based on action and state. There is a huge literature on the study of state machines and how to efficiently implement them; you might want to do some reading. \$\endgroup\$ Apr 13, 2020 at 23:37
  • 1
    \$\begingroup\$ Please don't fix the code in the question based on the answers you've been given, it makes it impossible for other readers to understand what the actual review was supposed to be. \$\endgroup\$
    – Flater
    Apr 15, 2020 at 14:32

6 Answers 6

27
+50
\$\begingroup\$

class/instance variables

In your code, you use class variables instead of instance variables.

You have to know that class variables are shared in all instance, for example:

class CoffeeMachine:
    water = 400


my_machine = CoffeeMachine()
your_machine = CoffeeMachine()

CoffeeMachine.water = 0
print(my_machine.water)
print(your_machine.water)

You get 0 in both machines!

The right way is to used instance variable. Instance variables determine the state of your object:

class CoffeeMachine:
    def __init__(self):
        self.water = 400


my_machine = CoffeeMachine()
your_machine = CoffeeMachine()

my_machine.water = 0
print(my_machine.water)
print(your_machine.water)

So, in your code, you can replace CoffeeMachine.sothing by self.sothing.

See the chapter Class and Instance Variables in the Python documentation.

Your constructor become:

class CoffeeMachine:
    def __init__(self):
        self.water = 400
        self.milk = 540
        self.coffee_beans = 120
        self.cups = 9
        self.money = 550
        self.running = False

infinite recursion

I have detected a potential infinite recursion:

  • The __init__ function calls `start``,
  • The start function calls on of the actions,
  • Each action calls return_to_menu,
  • And the return_to_menu function calls start again…

To avoid that, you can use an infinite loop, which will be controlled by the running attribute. Here is the scenario:

The machine is initialised: running is True,

While running is True:

  • The user enter the action he wants to do
  • The machine execute the action

You can easily translate into a main function:

def main():
    machine = CoffeeMachine()
    while machine.running:
        action = ask_action()
        machine.execute_action(action)


if __name__ == '__main__':
    main()

Of course, we need to change the implementation slightly:

  • the initialisation must set running to True,

    def __init__(self):
        ...
        self.running = False
    
  • the old start method is divided into 2 functions with a single role: prompting the user and running an action.

  • The return_to_menu is removed.

Prompting the user

When you ask something to the user you generally need to check the input to make sure it matches what we need. If not, we loop forever.

For the ask_action function, we have a set of acceptable answers: "buy", "fill", "take", "exit", "remaining". So, we can loop forever until the user enter an acceptable answer.

In Python, we can use an enumeration for that:

import enum

class Action(enum.Enum):
    BUY = "buy"
    FILL = "fill"
    TAKE = "take"
    EXIT = "exit"
    REMAINING = "remaining"

Here is a small demo of the possibilities:

>>> possible_values = [action.value for action in Action]
>>> possible_values
['buy', 'fill', 'take', 'exit', 'remaining']

>>> action = Action("fill")
>>> action
<Action.FILL: 'fill'>

>>> action = Action("quit")
Traceback (most recent call last):
  ...
ValueError: 'quit' is not a valid Action

Here is how you can define the ask_action function:

import enum

class Action(enum.Enum):
    BUY = "buy"
    FILL = "fill"
    TAKE = "take"
    EXIT = "exit"
    REMAINING = "remaining"

def ask_action():
    possible_values = ", ".join([action.value for action in Action])
    while True:
        answer = input(f"Write action ({possible_values}):\n")
        try:
            return Action(answer)
        except ValueError:
            print(f"This answer is not valid: {answer}")

Note: ask_action is a function here, there no need to turn it into a method since it doesn't access the class variables or methods.

executing an action

It easy to change the old start method into a execute_action method. This method has the parameter action:

def execute_action(self, action):
    if action == Action.BUY:
        self.buy()
    elif action == Action.FILL:
        self.fill()
    elif action == Action.TAKE:
        self.take()
    elif action == Action.EXIT:
        self.running = False
    elif action == Action.REMAINING:
        self.show_remaining()
    else:
        raise NotImplementedError(action)

The implementation is slightly changed:

  • The exit action is changed to set running to False.
  • NotImplementedError is raised if the action is unknown: this prevent unwanted behavior if your Action enumaration changes in the future but you forget to update execute_action.
  • status (which is renamed show_remaining) is fixed: no need to take the class in parameter.

As you can see, it is very simple.

Show remaining

The status function was renamed show_remaining to use a verb and match the term used in Action. But you can also change the Action into "status" if you prefer.

The status don't need to have any parameter because you only want to display the instance variable values. So, you can write:

def show_remaining(self):
    """
    Display the quantities of supplies in the machine at the moment
    """
    print(f"The coffee machine has:")
    print(f"{self.water} of water")
    print(f"{self.milk} of milk")
    print(f"{self.coffee_beans} of coffee beans")
    print(f"{self.cups} of disposable cups")
    print(f"${self.money} of money")

Instead of using a comment you can use a docstring. This is that way we document function and classes in Python.

You can read The Hitchhiker Guide to Python about docstring and API documentation in general. Very good book.

Ask for a drink

The "buy" action is similar to the "ask_action/execute_action". If you use the same logic, you'll see that you can drop or reimplement the deduct_supplies function too.

The difference is that you want the user to enter a number instead of a text. You have: 1 - "espresso", 2 - "latte", 3 - "cappuccino", for "back to main menu", you can choose 9. All that can be stored in a class Python dict to do the mapping between numbers and labels.

Note that ask_drink is a good name for this function:

def ask_drink():
    choices = {1: "espresso", 2: "latte", 3: "cappuccino", 9: "back to main menu"}
    possible_values = ", ".join(f"{value} - {name}" for value, name in sorted(choices.items()))
    while True:
        answer = input(f"What do you want to buy?  ({possible_values}):\n")
        try:
            value = int(answer)
            if value in choices:
                return value
            print(f"This answer is not valid: {answer}")
        except ValueError:
            print(f"This is not a number: {answer}")

Remarks:

  • sorted is required because dict keys are unordered (well, actually, recent versions of Python keep keys order),
  • Using value in choices is a good way to check if a key is in a dictionary.

Consumption (deduced supplies)

In your coffee machine, deduced supplies is represented as a list of 5 elements. For instance, we have [250, 0, 16, 1, 4] for water, milk, coffee beans, cups and money. If you have a list, you need to access the items by index. But I would be easier to access the items by name. To do that, you can use a collections.namedtuple. A namedtuple is a factory function which creates a class (a subclass of tuple).

First, you can define a new tuple class, we call it Consumption:

import collections

Consumption = collections.namedtuple("Consumption", "water, milk, coffee_beans, cups, money")

You can instanciate the Consumption like a classic tuple or with key/value pairs:

espresso_cons = Consumption(250, 0, 16, 1, 4)
latte_cons = Consumption(water=350, milk=75, coffee_beans=20, cups=1, money=7)
cappuccino_cons = Consumption(water=200, milk=100, coffee_beans=12, cups=1, money=6)

note: the second form is really more readable.

Checking availability

When you need to "check" something, you can think about exceptions. The idea behind this is: I do some tests and if something is wrong I raise an exception. The exception type and/or the exception message can detail the problem. Then I can use an exception handler to display the message.

To define an exception, a good practice is to inherit the Exception class like this:

class NotEnoughSupplyError(Exception):
    def __init__(self, supply):
        msg = f"Sorry, not enough {supply}"
        super(NotEnoughSupplyError, self).__init__(msg)

This exception take a supply parameter which is the name of the missing supply.

You can then implement the available_check method as below:

def available_check(self, consumption):
    """
    Checks if it can afford making that type of coffee at the moment

    :param consumption: the Consumption
    :raise NotEnoughSupplyError: if at least one supply is missing.
    """
    if self.water - consumption.water < 0:
        raise NotEnoughSupplyError("water")
    elif self.milk - consumption.milk < 0:
        raise NotEnoughSupplyError("milk")
    elif self.coffee_beans - consumption.coffee_beans < 0:
        raise NotEnoughSupplyError("coffee beans")
    elif self.cups - consumption.cups < 0:
        raise NotEnoughSupplyError("cups")

Really simple, isn't it?

The buy method

You know have all the elements in hand to implement the buy method:

def buy(self):
    drink = ask_drink()
    if drink == 9:
        return
    espresso_cons = Consumption(250, 0, 16, 1, 4)
    latte_cons = Consumption(water=350, milk=75, coffee_beans=20, cups=1, money=7)
    cappuccino_cons = Consumption(water=200, milk=100, coffee_beans=12, cups=1, money=6)
    consumption = {1: espresso_cons, 2: latte_cons, 3: cappuccino_cons}[drink]
    try:
        self.available_check(consumption)
    except NotEnoughSupplyError as exc:
        print(exc)
    else:
        print("I have enough resources, making you a coffee!")
        self.water -= consumption.water
        self.milk -= consumption.milk
        self.coffee_beans -= consumption.coffee_beans
        self.cups -= consumption.cups
        self.money += consumption.money

To get the consumption we introduce a small mapping between each drink value and each Consumption instances.

Of course, instead of an exception handler you can use a classic if. But I wanted to show you something powerful.

The fill method

Again, to implement the fill method, you can introduce a function ask_quantity which ask for a quantity of a given supply. This function take a message in parameter:

def ask_quantity(msg):
    while True:
        answer = input(msg + "\n")
        try:
            value = int(answer)
            if value >= 0:
                return value
            print(f"This answer is not valid: {answer}")
        except ValueError:
            print(f"This is not a number: {answer}")

The fill method can be implemented as follow:

def fill(self):
    """
    Add supplies to the machine
    """
    self.water += ask_quantity("Write how many ml of water do you want to add:")
    self.milk += ask_quantity("Write how many ml of milk do you want to add:")
    self.coffee_beans += ask_quantity("Write how many grams of coffee beans do you want to add:")
    self.cups += ask_quantity("Write how many disposable cups of coffee do you want to add:")

The take method.

Not sure to understand what the take method do: money is always reset to 0!?

Putting everything together

As you can see, I have done a lot of improvements. You can certainly go further, but write something simple and easy to read.

import collections
import enum


class Action(enum.Enum):
    BUY = "buy"
    FILL = "fill"
    TAKE = "take"
    EXIT = "exit"
    REMAINING = "remaining"


def ask_action():
    possible_values = ", ".join([action.value for action in Action])
    while True:
        answer = input(f"Write action ({possible_values}):\n")
        try:
            return Action(answer)
        except ValueError:
            print(f"This answer is not valid: {answer}")


def ask_drink():
    choices = {1: "espresso", 2: "latte", 3: "cappuccino", 9: "back to main menu"}
    possible_values = ", ".join(f"{value} - {name}" for value, name in sorted(choices.items()))
    while True:
        answer = input(f"What do you want to buy?  ({possible_values}):\n")
        try:
            value = int(answer)
            if value in choices:
                return value
            print(f"This answer is not valid: {answer}")
        except ValueError:
            print(f"This is not a number: {answer}")


def ask_quantity(msg):
    while True:
        answer = input(msg + "\n")
        try:
            value = int(answer)
            if value >= 0:
                return value
            print(f"This answer is not valid: {answer}")
        except ValueError:
            print(f"This is not a number: {answer}")


Consumption = collections.namedtuple("Consumption", "water, milk, coffee_beans, cups, money")


class NotEnoughSupplyError(Exception):
    def __init__(self, supply):
        msg = f"Sorry, not enough {supply}"
        super(NotEnoughSupplyError, self).__init__(msg)


class CoffeeMachine:
    def __init__(self):
        # quantities of items the coffee machine already had
        self.water = 400
        self.milk = 540
        self.coffee_beans = 120
        self.cups = 9
        self.money = 550
        self.running = True

    def execute_action(self, action):
        if action == Action.BUY:
            self.buy()
        elif action == Action.FILL:
            self.fill()
        elif action == Action.TAKE:
            self.take()
        elif action == Action.EXIT:
            self.running = False
        elif action == Action.REMAINING:
            self.show_remaining()
        else:
            raise NotImplementedError(action)

    def available_check(self, consumption):
        """
        Checks if it can afford making that type of coffee at the moment

        :param consumption: the Consumption
        :raise NotEnoughSupplyError: if at least one supply is missing.
        """
        if self.water - consumption.water < 0:
            raise NotEnoughSupplyError("water")
        elif self.milk - consumption.milk < 0:
            raise NotEnoughSupplyError("milk")
        elif self.coffee_beans - consumption.coffee_beans < 0:
            raise NotEnoughSupplyError("coffee beans")
        elif self.cups - consumption.cups < 0:
            raise NotEnoughSupplyError("cups")

    def buy(self):
        drink = ask_drink()
        if drink == 9:
            return
        espresso_cons = Consumption(250, 0, 16, 1, 4)
        latte_cons = Consumption(water=350, milk=75, coffee_beans=20, cups=1, money=7)
        cappuccino_cons = Consumption(water=200, milk=100, coffee_beans=12, cups=1, money=6)
        consumption = {1: espresso_cons, 2: latte_cons, 3: cappuccino_cons}[drink]
        try:
            self.available_check(consumption)
        except NotEnoughSupplyError as exc:
            print(exc)
        else:
            print("I have enough resources, making you a coffee!")
            self.water -= consumption.water
            self.milk -= consumption.milk
            self.coffee_beans -= consumption.coffee_beans
            self.cups -= consumption.cups
            self.money += consumption.money

    def fill(self):
        """
        Add supplies to the machine
        """
        self.water += ask_quantity("Write how many ml of water do you want to add:")
        self.milk += ask_quantity("Write how many ml of milk do you want to add:")
        self.coffee_beans += ask_quantity("Write how many grams of coffee beans do you want to add:")
        self.cups += ask_quantity("Write how many disposable cups of coffee do you want to add:")

    def take(self):
        """
        Take the money from the machine
        """
        print(f"I gave you ${self.money}")
        self.money = 0

    def show_remaining(self):
        """
        Display the quantities of supplies in the machine at the moment
        """
        print(f"The coffee machine has:")
        print(f"{self.water} of water")
        print(f"{self.milk} of milk")
        print(f"{self.coffee_beans} of coffee beans")
        print(f"{self.cups} of disposable cups")
        print(f"${self.money} of money")


def main():
    machine = CoffeeMachine()
    while machine.running:
        action = ask_action()
        machine.execute_action(action)


if __name__ == '__main__':
    main()

IMO, the money should not be a supply like water...

\$\endgroup\$
3
  • 1
    \$\begingroup\$ You might want to add in your first section that if not CoffeeMachine.running should become if not self.running. \$\endgroup\$
    – mkrieger1
    Apr 14, 2020 at 11:38
  • 2
    \$\begingroup\$ Regarding your section on "infinite recursion", I'd like to point out that one state tail calling the next state is a widely-used pattern to implement state machines. And, as this question here, and many other questions on Stack Overflow show, it also seems to be a very natural way of modeling them, for beginners like the OP. Also, as explained by Guy L. Steele and others, tail calls are fundamental to OOP. So, there is nothing inherently wrong with how the OP modeled the state machine, in fact, it is the standard OOP way to model a state machine. It's only a limitation of Python that prevents … \$\endgroup\$ Apr 15, 2020 at 15:48
  • \$\begingroup\$ … the solution from working. (Some people would argue it's not so much a limitation of Python but rather a limitation of Guido van Rossum, since there are plenty of Pythonistas and Python implementors who would like Proper Tail Calls in Python, but Guido van Rossum has explicitly forbidden any Python implementation from providing them.) \$\endgroup\$ Apr 15, 2020 at 15:50
14
\$\begingroup\$

When you have multiple if statements, like your code does, it can be an indication that you can use a visitor pattern to your code. I will use a dict in my example.

Your code:

def start(self):
    self.running = True
    self.action = input("Write action (buy, fill, take, remaining, exit):\n")
    print()
    #possible choices to perform in the coffee machine
    if self.action == "buy":
        self.buy()
    elif self.action == "fill":
        self.fill()
    elif self.action == "take":
        self.take()
    elif self.action == "exit":
        exit()
    elif self.action == "remaining":
        self.status()

Re-written, using the visitor pattern:

def action_buy(self):
    self.buy()

action_choices = { "buy" : action_buy,
                   "fill" : action_fill, ...

def start(self):
    self.running = True
    self.action = input("Write action (buy, fill, take, remaining, exit):\n")
    print()
    #possible choices to perform in the coffee machine
    if self.action in action_choices:
        action_choices[self.action](self)

You can use the same principle on the function buy. I didn't verify the code so probably there are some errors, but hope you got the idea.

\$\endgroup\$
2
  • \$\begingroup\$ that is really useful, thanks \$\endgroup\$
    – DeltaHaxor
    Apr 12, 2020 at 20:45
  • 3
    \$\begingroup\$ Even better, you can use the keys of the dict to generate your prompt text: input("Write action ({actions}):\n".format(actions=action_choices.keys()) \$\endgroup\$
    – Kroltan
    Apr 13, 2020 at 17:11
8
\$\begingroup\$

You should split business logic and user interface.

Whilst it's common to get coffee machines that are hella advanced and can talk to humans. When programming you should build in layers.

I always build the core of the logic to be a generic as possible. This allows for easier tests, re-usability and segregation of the project.

This would means changing the CoffeeMachine to only contain available and deduct as methods.

I would also add a class CoffeeInterface that can be a cmd.Cmd. This will help cut out some of the code that you have right now.

  • Only use self.foo for attributes defined in the __init__. Everything else should be passed via arguments.

    I also wouldn't change any of the attributes defined in the __init__ as only things directly related to the class should be defined there.

  • Please don't do things like if not CoffeeMachine.running: self.start() in __init__. You should let the user call .start().

  • exit() is not intended to be used in actual live programs. Instead you should structure your code so that it is not needed.

    There are times when exit(1) or raise SystemExit(1) are useful. But unless you're doing Unix programming it's unlikely you'll need these.

All this together can get the following code. Not much has changed as I mostly just split the two classes.

class CoffeeMachine:
    def __init__(self, water, milk, coffee_beans, cups, money):
        self.water = water
        self.milk = milk
        self.coffee_beans = coffee_beans
        self.cups = cups
        self.money = money

    def available(self, water, milk, coffee_beans, cups, _):
        not_available = ""
        if self.water - water < 0:
            not_available = "water"
        elif self.milk - milk < 0:
            not_available = "milk"
        elif self.coffee_beans - coffee_beans < 0:
            not_available = "coffee beans"
        elif self.cups - cups < 0:
            not_available = "disposable cups"

        if not_available != "":
            print(f"Sorry, not enough {not_available}!")
            return False
        else:
            print("I have enough resources, making you a coffee!")
            return True

    def deduct(self, water, milk, coffee_beans, cups, money):
        self.water -= water
        self.milk -= milk
        self.coffee_beans -= coffee_beans
        self.cups -= cups
        self.money += money


class CoffeeInterface(cmd.Cmd):
    def __init__(self, coffee_machine, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.coffee_machine = coffee_machine

    def do_buy(self, _):
        choice = input("What do you want to buy? 1 - espresso, 2 - latte, 3 - cappuccino, back - to main menu:\n")
        if choice == '1':
            requirements = [250, 0, 16, 1, 4]
            if self.coffee_machine.available(*requirements):
                self.coffee_machine.deduct(*requirements)

        elif choice == '2':
            requirements = [350, 75, 20, 1, 7]
            if self.coffee_machine.available(*requirements):
                self.coffee_machine.deduct(*requirements)

        elif choice == "3":
            requirements = [200, 100, 12, 1, 6]
            if self.coffee_machine.available(*requirements):
                self.coffee_machine.deduct(*requirements)

        elif choice == "back": # if the user changed his mind
            pass

    def do_fill(self, _):
        """Add supplies to the machine."""
        self.coffee_machine.water += int(input("Write how many ml of water do you want to add:\n"))
        self.coffee_machine.milk += int(input("Write how many ml of milk do you want to add:\n"))
        self.coffee_machine.coffee_beans += int(input("Write how many grams of coffee beans do you want to add:\n"))
        self.coffee_machine.cups += int(input("Write how many disposable cups of coffee do you want to add:\n"))

    def do_take(self, _):
        """Take money from the machine."""
        print(f"I gave you ${self.coffee_machine.money}")
        self.coffee_machine.money -= self.coffee_machine.money

    def do_status(self):
        """Display the quantities of supplies in the machine at the moment."""
        print(f"The coffee machine has:")
        print(f"{self.coffee_machine.water} of water")
        print(f"{self.coffee_machine.milk} of milk")
        print(f"{self.coffee_machine.coffee_beans} of coffee beans")
        print(f"{self.coffee_machine.cups} of disposable cups")
        print(f"${self.coffee_machine.money} of money")


CoffeeInterface(CoffeeMachine(400, 540, 120, 9, 550)).cmdloop()

Now that the two separate things have been split apart we can focus on reviewing the code.

  • I would move supplies into yet another class.
    I would make this class a named tuple as it has a couple of benefits:

    1. It's immutable, meaning that it's hard to mess up CoffeeMachine.available.
    2. Getting specific values from it is clean, reduced.water rather than reduced[0].
    3. We can pass around one object rather than using nasty *requirements.

    I have elected to use typing.NamedTuple however collections.namedtuple may be simpler to understand as it doesn't use type hints.

  • I would define the __sub__ dunder method on the Supplies class.
    This means when we are subtracting supplies, the core of your code, it's nicer to the eyes.

    To get this working correctly you have the option to make money work differently to the rest. Or you can make money negative when you're supplying the cost of a drink. I think a negative cost to make a drink is the most intuitive.

  • The code for available can be reduced to less lines, however this would make the code more confusing at the moment.

  • I would move the print out of the available function, it is better located in do_buy.
    To allow printing the missing item you can change its name to unavailable and return the unavailable item. This would have the benefit of still making sense.

  • You should move the available drinks out of do_buy.
    If you move them into a dictionary then you can significantly reduce the amount of code in do_buy.

    To do this we can build a dictionary with each key being the value 1, 2 or 3. And the value as the Supplies for that drink. From here we can use dict.get(choice, None), which will return the Supplies for the selected drink or None if the user didn't enter a valid choice.

    From here we can just return if it's not a valid choice, and interact with the CoffeeMachine otherwise.

  • To simplify do_fill and take we can add the __add__ dunder method.
    This means we only need one + rather than four.

import cmd
from typing import NamedTuple


class Supplies(NamedTuple):
    water: int
    milk: int
    coffee_beans: int
    cups: int
    money: int

    def __sub__(self, other):
        return Supplies(
            self.water - other.water,
            self.milk - other.milk,
            self.coffee_beans - other.coffee_beans,
            self.cups - other.cups,
            self.money - other.money,
        )

    def __add__(self, other):
        return Supplies(
            self.water + other.water,
            self.milk + other.milk,
            self.coffee_beans + other.coffee_beans,
            self.cups + other.cups,
            self.money + other.money,
        )


DRINKS = {
    '1': Supplies(250, 0, 16, 1, -4),
    '2': Supplies(350, 75, 20, 1, -7),
    '3': Supplies(200, 100, 12, 1, -6),
}


class CoffeeMachine:
    def __init__(self, supplies):
        self.supplies = supplies

    def unavailable(self, drink):
        remaining = self.supplies - drink
        not_available = ""
        if remaining.water < 0:
            not_available = "water"
        elif remaining.milk < 0:
            not_available = "milk"
        elif remaining.coffee_beans < 0:
            not_available = "coffee beans"
        elif remaining.cups < 0:
            not_available = "disposable cups"
        return not_available if not_available else None

    def deduct(self, drink):
        self.supplies -= drink


class CoffeeInterface(cmd.Cmd):
    def __init__(self, coffee_machine, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.coffee_machine = coffee_machine

    def do_buy(self, _):
        choice = input("What do you want to buy? 1 - espresso, 2 - latte, 3 - cappuccino, back - to main menu:\n")
        drink = DRINKS.get(choice, None)
        if drink is None:
            return

        unavailable = self.coffee_machine.available(drink)
        if unavailable:
            print(f"Sorry, not enough {unavailable}!")
        else:
            print("I have enough resources, making you a coffee!")
            self.coffee_machine.deduct(drink)

    def do_fill(self, _):
        """Add supplies to the machine."""
        self.coffee_machine.supplies += Supplies(
            int(input("Write how many ml of water do you want to add:\n")),
            int(input("Write how many ml of milk do you want to add:\n")),
            int(input("Write how many grams of coffee beans do you want to add:\n")),
            int(input("Write how many disposable cups of coffee do you want to add:\n")),
            0,
        )

    def do_take(self, _):
        """Take money from the machine."""
        money = self.coffee_machine.supplies.money
        print(f"I gave you ${money}")
        self.coffee_machine.supplies -= Supplies(0, 0, 0, 0, money)

    def do_status(self):
        """Display the quantities of supplies in the machine at the moment."""
        supplies = self.coffee_machine.supplies
        print(f"The coffee machine has:")
        print(f"{supplies.water} of water")
        print(f"{supplies.milk} of milk")
        print(f"{supplies.coffee_beans} of coffee beans")
        print(f"{supplies.cups} of disposable cups")
        print(f"${supplies.money} of money")


CoffeeInterface(CoffeeMachine(Supplies(400, 540, 120, 9, 550))).cmdloop()
  • Given the amount of self.coffee_machine.supplies.{x} it should be blindingly apparent that CoffeeMachine is now more of a hindrance than a help.

    • Having to read and write self.coffee_machine.supplies is rather annoying.
    • We can easily change deduct to just self.coffee_machine.supplies -= drink.
    • The function unavailable can be moved onto either Supplies or CoffeeInterface.
  • One of the benefits to using NamedTuple is that it defines a means to iterate over it without us having to write it.
    This means that we can simplify the __sub__, __add__ and unavailable methods.

    To do so we can use zip which lets us iterate over two things at the same time.

    foos = 'abcdef'
    bars = 'ghijkl'
    
    # non-zip
    for i in range(len(foos)):
        print(foos[i], bars[i])
    
    # zip
    for foo, bar in zip(foos, bars):
        print(foo, bar)
    

    We can also use a list comprehension or generator expression to build the new Supplies on one line.

  • With cmd you can pass a string when entering a command. This means it is possible to enter buy espresso.

    It would be cool if you used this to buy by an item's name rather than an odd number.

    To allow this you can have a menu option that shows a list of items that you can buy.

import cmd
from typing import NamedTuple


class Supplies(NamedTuple):
    water: int
    milk: int
    coffee_beans: int
    cups: int
    money: int

    def __sub__(self, other):
        return Supplies(*[s - o for s, o in zip(self, other)])

    def __add__(self, other):
        return Supplies(*[s + o for s, o in zip(self, other)])

    def unavailable(self):
        return [
            field
            for field, value in zip(self._fields, self)
            if value < 0
        ]


DRINKS = {
    'espresso': Supplies(250, 0, 16, 1, -4),
    'latte': Supplies(350, 75, 20, 1, -7),
    'cappuccino': Supplies(200, 100, 12, 1, -6),
}


class CoffeeInterface(cmd.Cmd):
    def __init__(self, supplies, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.supplies = supplies

    def do_menu(self, _):
        print('\n'.join(DRINKS))

    def do_buy(self, choice):
        drink = DRINKS.get(choice.lower(), None)
        if drink is None:
            return

        remaining = self.supplies - drink
        unavailable = remaining.unavailable()
        if unavailable:
            print(f"Sorry, not enough {', '.join(unavailable)}!")
        else:
            print("I have enough resources, making you a coffee!")
            self.supplies = remaining

    def do_fill(self, _):
        """Add supplies to the machine."""
        self.supplies += Supplies(
            int(input("Write how many ml of water do you want to add:\n")),
            int(input("Write how many ml of milk do you want to add:\n")),
            int(input("Write how many grams of coffee beans do you want to add:\n")),
            int(input("Write how many disposable cups of coffee do you want to add:\n")),
            0,
        )

    def do_take(self, _):
        """Take money from the machine."""
        print(f"I gave you ${self.supplies.money}")
        self.supplies -= Supplies(0, 0, 0, 0, self.supplies.money)

    def do_status(self):
        """Display the quantities of supplies in the machine at the moment."""
        print(f"The coffee machine has:")
        print(f"{self.supplies.water} of water")
        print(f"{self.supplies.milk} of milk")
        print(f"{self.supplies.coffee_beans} of coffee beans")
        print(f"{self.supplies.cups} of disposable cups")
        print(f"${self.supplies.money} of money")


CoffeeInterface(Supplies(400, 540, 120, 9, 550)).cmdloop()
\$\endgroup\$
5
\$\begingroup\$

First some basic observations:

  1. Your running variable is shared across all CoffeeMachine objects -- as soon as you create one CoffeeMachine, it starts itself, and then any subsequent CoffeeMachine you create anywhere in the universe is also "running" and so it doesn't start itself! Unless this is a property you intended it to have (it would certainly not match the behavior of real-world coffee machines), you should make running an instance variable (i.e. put it in the __init__), or better yet, not have it at all (since you immediately initialize it anyway and never use it for anything else thereafter -- if a machine is always "running" as soon as it's created, no point in having a bool to indicate that state).

  2. Some of your instance variables are created after __init__. Python lets you do this, but it's considered bad practice because it's very easy to have bugs where you access a variable before it's initialized. In general, all instance variables should be declared in __init__.

  3. Your self.reduced variable is only used for available_check and deduct_supplies, which are called immediately after reduced is set -- reduced should simply be a parameter. If it's a parameter, then you know that its value doesn't matter after those functions return (which is the case) and you don't need to worry about what other parts of your code setting self.reduced might affect. The general rule here is that state should be as "short-lived" and/or "narrowly scoped" as possible. (edit: as I go through the rest of the code, I see that there's a common pattern of assigning values to self where a locally-scoped value would suffice. Never make data more persistent than it needs to be!)

Now, some more "big picture" notes on the structure of the methods:

  1. All of your actions call back to return_to_menu which calls back to start. Maybe start should just loop? That way return_to_menu doesn't need to get called at the end of every action method, and it's also obvious to someone reading your start method that it's actually a loop (you want the way your code works to be obvious to everyone who reads it).

  2. Specifying the different types of objects as Enums makes it a little easier to keep track of the possible values and keep different parts of your code from having different versions of them.

  3. When you have associations between different pieces of data (like "type of ingredient" and "quantity"), a natural way to store that is in a dictionary. Again, this makes it easier to keep track of things, and it also makes it easier to say "do this for every ingredient" without having to copy and paste.

I took a few passes over this code seeing if I could convert everything into enums and dictionaries, with the general goal of never having to copy+paste the same word in multiple places, and turning all those if...elif chains into iterations or lookups. The general pattern I've followed is to have the "name" of the enumeration be the way you refer to it in your code and the "value" be the user-visible rendering (which is usually but not always the same); in real life you'd probably have a slightly more complex (and extensible) mapping that would allow for localization, etc, but as a general demonstration of the concept I think this is good enough.

Here's what I came up with; there's a lot more data declared up front that defines how the coffee machine operates, and a lot less actual code in the methods.

from enum import Enum, auto
from typing import Dict, List

class Inventory(Enum):
    """Inventory items."""
    water = "water"
    milk = "milk"
    coffee_beans = "coffee beans"
    cups = "disposable cups"
    money = "money"

# The unit description of each inventory item.
UNITS = { 
    Inventory.water: "ml of",
    Inventory.milk: "ml of",
    Inventory.coffee_beans: "grams of",
    Inventory.cups: "of",
    Inventory.money: "of",
}

class Action(Enum):
    """Menu actions."""
    buy = "buy"
    fill = "fill"
    take = "take"
    status = "remaining"

class Product(Enum):
    """Products for sale."""
    espresso = "1"
    latte = "2"
    cappuccino = "3"

# The cost of each product.
COSTS = { 
    Product.espresso: {
        Inventory.water: 250,
        Inventory.milk: 0,
        Inventory.coffee_beans: 16,
        Inventory.cups: 1,
        Inventory.money: 4,
    },
    Product.latte: {
        Inventory.water: 350,
        Inventory.milk: 75,
        Inventory.coffee_beans: 20,
        Inventory.cups: 1,
        Inventory.money: 7,
    },
    Product.cappuccino: {
        Inventory.water: 200,
        Inventory.milk: 100,
        Inventory.coffee_beans: 12,
        Inventory.cups: 1,
        Inventory.money: 6,
    },
}

class CoffeeMachine:

    def __init__(
        self, 
        water: int, 
        milk: int, 
        coffee_beans: int, 
        cups: int, 
        money: int
    ):
        self.quantities = {
            Inventory.water: water,
            Inventory.milk: milk,
            Inventory.coffee_beans: coffee_beans,
            Inventory.cups: cups,
            Inventory.money: money,
        }

        self.run()

    def run(self) -> None:
        do_action = {
            Action.buy: self.buy,
            Action.fill: self.fill,
            Action.take: self.take,
            Action.status: self.status,
        }
        actions = ', '.join(action.value for action in Action)

        while True:
            action = input(f"Write action ({actions}, exit):\n")
            print()
            if action == "exit":
                break
            do_action[Action(action)]()
            print()

    def available_check(self, cost: Dict[Inventory, int]) -> bool:
        """checks if it can afford making that type of coffee at the moment"""
        for item in Inventory:
            if self.quantities[item] < cost[item]:
                print(f"Sorry, not enough {item.value}!")
                return False
        else:
            print("I have enough resources, making you a coffee!")
            return True

    def deduct_supplies(self, cost: Dict[Inventory, int]) -> None:
        """performs operation from the cost list, based on the coffee chosen"""
        for item in Inventory:
            self.quantities[item] -= cost[item]

    def buy(self) -> None:
        products = ", ".join(
            f"{product.value} - {product.name}" for product in Product
        )
        choice = input(
            f"What do you want to buy? {products}, back - to main menu:\n"
        )
        if choice == "back":
            return
        cost = COSTS[Product(choice)]
        if self.available_check(cost):
            self.deduct_supplies(cost)

    def fill(self) -> None: 
        """for adding supplies to the machine"""
        for item in Inventory:
            if item == Inventory.money:
                continue
            self.quantities[item] += int(input(
                "Write how many "
                f"{UNITS[item]} {item.value}"
                " do you want to add:\n"
            ))

    def take(self) -> None:
        """for taking the money from the machine"""
        print(f"I gave you ${self.quantities[Inventory.money]}")
        self.quantities[Inventory.money] = 0

    def status(self) -> None: 
        """display the quantities of supplies in the machine at the moment"""
        print(f"The coffee machine has:")
        for item in Inventory:
            print(f"{self.quantities[item]} {UNITS[item]} {item.value}")

# specify the quantities of supplies at the beginning
# water, milk, coffee beans, disposable cups, money
CoffeeMachine(400, 540, 120, 9, 550) 
\$\endgroup\$
1
  • \$\begingroup\$ Wow, that's a lot of stuff I doubt the user will ever need. You may want to post this as a separate question as from a 2 second glance I'm not sure it's that great. Lots of PEP 8, PEP 257 and Enum violations here too. \$\endgroup\$
    – Peilonrayz
    Apr 13, 2020 at 0:23
4
\$\begingroup\$

Instead of

    def start(self):
        self.running = True
        self.action = input("Write action (buy, fill, take, remaining, exit):\n")
        print()
        #possible choices to perform in the coffee machine
        if self.action == "buy":
            self.buy()
        elif self.action == "fill":
            self.fill()
        elif self.action == "take":
            self.take()
        elif self.action == "exit":
            exit()
        elif self.action == "remaining":
            self.status()

I would suggest

  def start(self):
    self.running = True

    action = input("Write action (buy, fill, take, remaining, exit):\n")
    print()

    try:
      getattr(self, self.action)()
    except AttributeError:
      print("Invalid action")   

but you then have to add methods exit(self) and remaining(self).

\$\endgroup\$
1
\$\begingroup\$

You might gain more OOP points by using more objects.

Start by defining an exception:

class NotAvailable(Exception):
   pass

When something runs out, you can raise the exception and have the program return cleanly to the menu. This simplifies the flow control.

Next, define a worker which will actually make the coffee etc:

class Worker(object):
    def __init__(self):
      pass

    def list_actions(self):
      return ['buy', 'fill', 'take', 'remaining']

    def set_state(self,water,milk,coffee_beans,cups,money ):
        # quantities of items the coffee machine already had
        self.water = water
        self.milk = milk
        self.coffee_beans = coffee_beans
        self.cups = cups
        self.money = money

    def available_check(self): # checks if it can afford making that type of coffee at the moment
        self.not_available = "" # by checking whether the supplies goes below 0 after it is deducted
        if self.water - self.reduced[0] < 0:
            self.not_available = "water"
        elif self.milk - self.reduced[1] < 0:
            self.not_available = "milk"
        elif self.coffee_beans - self.reduced[2] < 0:
            self.not_available = "coffee beans"
        elif self.cups - self.reduced[3] < 0:
            self.not_available = "disposable cups"

        if self.not_available != "": # if something was detected to be below zero after deduction
            print(f"Sorry, not enough {self.not_available}!")
            raise NotAvailable

        else: # if everything is enough to make the coffee
            print("I have enough resources, making you a coffee!")
            return True

    def deduct_supplies(self):
# performs operation from the reduced list, based on the coffee chosen
        self.water -= self.reduced[0]
        self.milk -= self.reduced[1]
        self.coffee_beans -= self.reduced[2]
        self.cups -= self.reduced[3]
        self.money += self.reduced[4]

    def buy(self):
        self.choice = input("What do you want to buy? 1 - espresso, 2 - latte, 3 - cappuccino, back - to main menu:\n")
        if self.choice == '1':
            self.reduced = [250, 0, 16, 1, 4] # water, milk, coffee beans, cups, money
            self.available_check() # checks if supplies are available
            self.deduct_supplies() # if it is, then it deducts

        elif self.choice == '2':
            self.reduced = [350, 75, 20, 1, 7]
            self.available_check()
            self.deduct_supplies()

        elif self.choice == "3":
            self.reduced = [200, 100, 12, 1, 6]
            self.available_check() 
            self.deduct_supplies()       

        elif self.choice != 'back':
            print ("Choice not recognised")


    def fill(self): # for adding supplies to the machine
        self.water += int(input("Write how many ml of water do you want to add:\n"))
        self.milk += int(input("Write how many ml of milk do you want to add:\n"))
        self.coffee_beans += int(input("Write how many grams of coffee beans do you want to add:\n"))
        self.cups += int(input("Write how many disposable cups of coffee do you want to add:\n"))

    def take(self): # for taking the money from the machine
        print(f"I gave you ${self.money}")
        self.money -= self.money

    def remaining(self): # to display the quantities of supplies in the machine at the moment
        print(f"The coffee machine has:")
        print(f"{self.water} of water")
        print(f"{self.milk} of milk")
        print(f"{self.coffee_beans} of coffee beans")
        print(f"{self.cups} of disposable cups")
        print(f"${self.money} of money")

This is mainly your code, but I have change available_check() to raise the exception defined above, and removed the return_to_menu() method because the worker will simply finish working when it is finished.

Finally, the machine itself:

class CoffeeMachine(object):
  def __init__(self, water,milk,coffee_beans,cups,money ):
     """The coffee machine starts itself on initialisation.
         When running, it asks the user to select a task, and then passes the task to the worker.
     """

     self.worker = Worker()
     self.worker.set_state(water,milk,coffee_beans,cups,money)
     self.start()

  def start(self):
    """Start the machine running.
       Continue running until exit is requested
    """

    self.running = True
    while self.running:
      action = input("Write action (%s) or exit:\n" % ', '.join( self.worker.list_actions() ) )
      if action == 'exit':
        self.running = False
      elif action in self.worker.list_actions():
        self.execute_task(action)
      else:
        print ("INVALID OPTION -- PLEASE TRY AGAIN")

  def execute_task(self,action):
    """Execute a task, calling the worker method named in the action variable.
       The NotAvailable exception is caught
    """

    try:
      return getattr( self.worker, action)()
    except NotAvailable:
      print ("Please make another choice (%s not available)" % self.worker.not_available)

cm = CoffeeMachine(400, 540, 120, 9, 550)

Defining the worker as a separate object gives a clearer separation between the different tasks in your programming challenge.

\$\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.