Simulating an OOP Coffee Machine

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.

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

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) 

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

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.