Simple bank-atm system design OOP

Question

For practicing designing systems I created a simple bank ATM system.

I didn't want to make it to complicated so I left out things like cash bin per ATM and opening cash trays, etc. I also wanted to neglect APIs to validate pins, etc. (it's just a function that will always return True). I wanted to focus on classes and methods.

So in my example I have

• an Account class which has an account number, a balance and withdraw and deposit functions.
• a Card class which has accounts and a select account function which will search for the account number on the card and then return it.
• an ATM class which has the following variables: card_inserted, card_validated, current_account, current_card and the main function are perform_request (which will either give out the balance, deposit or withdraw money), validate_pin (which will make the call to the API, mocked in my code), select_account (which will select an account from the card)

I did write a test for it and it worked. I was wondering if I could get some feedback on that (not the test, I know I can do lot's of things to make the tests better but that was just a quick and dirty version of it)?

atm.py:

def validate_api(card_nr, pin):
 # validate account nr and pin
 # return true or fale
 return True
class Card():
 def __init__(self, accounts = []):
 self.accounts = accounts
 def select_account(self, account_nr):
 # check if there is a built in filter func in python
 for account in self.accounts:
 if account.account_nr == account_nr:
 return account
 raise Exception("Account number invalid")
class Account():
 def __init__(self, account_nr = None):
 self.account_nr = account_nr
 self.balance = 0
 def deposit(self, amount):
 self.balance += amount
 def withdraw(self, amount):
 self.balance -= amount
class ATM():
 def __init__(self):
 self.card_inserted = False
 self.card_validated = False
 self.current_account = None
 self.current_card = None
 def validate_card_and_pin(self):
 if not self.card_inserted:
 raise Exception('Card must be inserted first')
 if self.card_validated is False:
 raise Exception
 # raise Exception("Card not validated")
 def insert_card(self, bank_card):
 # code to accept card
 self.card_inserted = True
 self.current_card = bank_card
 def eject_card(self):
 self.end_session()
 # code to push card out
 def validate_pin(self, pin):
 if not self.card_inserted:
 raise Exception('Card must be inserted first')
 # card is inserted, pin has to be validated
 # post pin and card number to api
 # response will be saved in validated variable
 validated = validate_api(card_nr=0,pin=0)
 if validated == False:
 self.card_validated = False
 return self.card_validated
 self.card_validated = True
 return self.card_validated
 def select_account(self, account_nr):
 self.validate_card_and_pin()
 if self.current_card is None:
 raise Exception("no card in ATM")
 if self.card_validated is False:
 raise Exception("card not validated")
 current_account = self.current_card.select_account(account_nr)
 self.current_account = current_account
 '''
 1 = check balance
 2 = deposit money
 3 = withdraw money
 '''
 def perform_request(self, request, amount = 0):
 self.validate_card_and_pin()
 if request == 1:
 return self.check_balance()
 elif request == 2:
 return self.accept_cash(amount)
 elif request == 3:
 return self.give_out_cash(amount)
 else:
 raise Exception("invalid request")
 def accept_cash(self, amount):
 # open cash tray
 # count cash
 self.current_account.deposit(amount)
 return amount
 def give_out_cash(self, amount):
 # count cash
 # open tray
 self.current_account.withdraw(amount)
 return amount
 def check_balance(self):
 return self.current_account.balance
 def end_session(self):
 self.card_inserted = False
 self.card_validated = False
 self.current_account = None
 self.current_card = None
 return True

And here the test:

 def test_depositing_50_and_withdrawing_20():
 checking = Account(1)
 saving = Account(2)
 bank_card = Card([checking, saving])
 atm = ATM()
 
 atm.insert_card(bank_card)
 atm.validate_pin("123")
 atm.select_account(1)
 atm.perform_request(2, 50)
 if atm.perform_request(1) is not 50:
 raise Exception("depositing function doesn't work")
 atm.end_session()
 
 atm.insert_card(bank_card)
 atm.validate_pin("123")
 atm.select_account(1)
 atm.perform_request(3, 20)
 if atm.perform_request(1) is not 30:
 raise Exception("withdrawing function doesn't work")
 
 atm.select_account(2)
 atm.validate_pin("123")
 atm.perform_request(2, 10)
 
 if atm.perform_request(1) is not 10:
 raise Exception("depositing function doesn't work")
 
 print("Test successful") 

• Thomas: On another level I would like to add that banks don't keep track of a balance by editing a single value. Instead they use a form of event sourcing to keep track of all changes as well and use the event sourcing to calculate what the resulting balance should be.

Answer 1

class Card():

You don't need parenthesis in the class declarations. Also, while most of your formatting is pretty good, there's a couple nitpicky things. When supplying a default argument, there shouldn't be spaces around the =, and there should be two blank lines between top-level definitions.

---

You should really never have a mutable default parameter like you do in:

def __init__(self, accounts = []):

In this case, since you never modify self.accounts, you're safe. If you ever added an add_account method that associated an account with a card by appending to self.accounts, you'd see that every Card in your program that was created using the default argument would change when that method is run.

I'd change it to:

def __init__(self, accounts=None): # Default to None
 self.accounts = accounts or [] # then create the list inside the method instead

---

# check if there is a built in filter func in python

There are multiple ways of using some fancy shortcut function to find the first element that satisfies a predicate, but honestly, I'd just stick to what you have. A for loop with an early return is pretty easy to understand. Since you want to raise an exception if an account isn't found, the other ways get a little bit sloppy. If you were fine having it return None on an error, you could use:

def select_account(self, account_nr):
 return next((account for account in self.accounts if account.account_nr == account_nr), None)

next attempts to grab the first element of an iterator that you give it. I'm giving it a generator expression, that will produce an element only if account.account_nr == account_nr is true. The None as the second argument is a default value if nothing is found. I still prefer the iterative for style though.

---

In Account, you're allowing None to be used as an account number. This strikes me as the kind of field that should not be "nullable", or be allowed to be omitted when creating an object. I think a (unique, existing) account number is pretty fundamental to the idea of a bank account. I would get rid of the default and force the user to supply that information when creating an account. It may be helpful though to have a second starting_balance parameter that lets the user to set the starting account balance, and allow that to default to 0.

---

validate_card_and_pin is a misleading name. It doesn't seem to do the validations. It expects that the validations have already been done and that the internal self.card_validated state is already set. assert_is_validated may be a better name for what it's doing.

---

I think validate_pin could be made clearer. Note how a lot of the code at the bottom is duplicated. You're setting card_validated to whatever validated is, then returning that value. The function can be simply:

def validate_pin(self, pin):
 if not self.card_inserted:
 raise Exception('Card must be inserted first')
 
 self.card_validated = validate_api(card_nr=0,pin=0)
 return self.card_validated

---

I'd maybe rethink throwing exceptions from the methods. In my eyes, a PIN being entered wrong for example isn't really exceptional. I'm a fan of returning None as a error indicator in cases where a function can only fail in one way; like validate_pin. You just have to be in the habit of identifying when a function returns None, and handling that case properly.

If you do want to use exceptions though, throwing a plain Exception is a bad idea. This makes it more difficult for the caller to catch and handle specifically exceptions from your code. I think this is an appropriate case to create custom exceptions. Something like:

class PINValidationFailed(RuntimeError):
 pass

Then the caller can catch specifically that exception to handle PIN failures.

---

'''
1 = check balance
2 = deposit money
3 = withdraw money
'''
def perform_request(self, request, amount = 0):

If that's intended as a docstring, it needs to be inside the function and indented:

def perform_request(self, request, amount=0):
 '''
 1 = check balance
 2 = deposit money
 3 = withdraw money
 '''

---

I think if self.card_validated is False: is clearer as simply if not self.card_validated:.

---

After touching this up, I'm left with:

def validate_api(card_nr, pin):
 # validate account nr and pin
 # return true or false
 return True
class Card:
 def __init__(self, accounts=None):
 self.accounts = accounts or []
 def select_account(self, account_nr):
 return next((account for account in self.accounts if account.account_nr == account_nr), None)
class Account:
 def __init__(self, account_nr, starting_balance=0):
 self.account_nr = account_nr
 self.balance = starting_balance
 def deposit(self, amount):
 self.balance += amount
 def withdraw(self, amount):
 self.balance -= amount
class ATM:
 def __init__(self):
 self.card_inserted = False
 self.card_validated = False
 self.current_account = None
 self.current_card = None
 def validate_card_and_pin(self):
 if not self.card_inserted:
 raise RuntimeError('Card must be inserted first')
 if not self.card_validated:
 raise RuntimeError("Card not validated")
 def insert_card(self, bank_card):
 # code to accept card
 self.card_inserted = True
 self.current_card = bank_card
 def eject_card(self):
 self.end_session()
 # code to push card out
 def validate_pin(self, pin):
 if not self.card_inserted:
 raise RuntimeError('Card must be inserted first')
 self.card_validated = validate_api(card_nr=0, pin=0)
 return self.card_validated
 def select_account(self, account_nr):
 self.validate_card_and_pin()
 if self.current_card is None:
 raise RuntimeError("no card in ATM")
 if self.card_validated is False:
 raise RuntimeError("card not validated")
 current_account = self.current_card.select_account(account_nr)
 self.current_account = current_account
 def perform_request(self, request, amount=0):
 '''
 1 = check balance
 2 = deposit money
 3 = withdraw money
 '''
 self.validate_card_and_pin()
 if request == 1:
 return self.check_balance()
 elif request == 2:
 return self.accept_cash(amount)
 elif request == 3:
 return self.give_out_cash(amount)
 else:
 raise RuntimeError("invalid request")
 def accept_cash(self, amount):
 # open cash tray
 # count cash
 self.current_account.deposit(amount)
 return amount
 def give_out_cash(self, amount):
 # count cash
 # open tray
 self.current_account.withdraw(amount)
 return amount
 def check_balance(self):
 return self.current_account.balance
 def end_session(self):
 self.card_inserted = False
 self.card_validated = False
 self.current_account = None
 self.current_card = None
 return True

• Tom: dude, thank you so much! this is exactly what I wanted to know, I don't have much experience with stuff like that, thank you for taking the time to answer so detail oriented, especially for the nitpicking syntax stuff! I was also thinking if there is a better way to do assert_is_validated, was reading about decorators, is this something I could use here?
• Carcigenicate: @Tom No problem. Glad to help. Hopefully someone with a more OOP background can make comments about that aspect.
• Carcigenicate: @Tom If you want to have it set up where you're just checking the state of the object before allowing something to happen, I don't think having an assert-thing function at the top of the method is necessarily bad. I've certainly done it, and I've seen it done in standard libraries.
• Carcigenicate: @Tom Here is actually an example of me doing it awhile ago (check-gene-compatibility). Note I use it in breed right below that.