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

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