10
\$\begingroup\$

I'm relatively new to python and coding in general, and I decided that this would be a good little practice project. This was also my first project involving classes and objects so I kept their implementation a little basic just to get the feel of it. I would appreciate any constructive criticism on how I could improve things like my readability, efficiency, and if there were easier ways to do things.

The intended output of the program is to simulate interacting with a very basic bank or ATM. I had no intention of actually storing any account information in a separate file, so each account pin is completely arbitrary and only serves as a medium to make the simulation a little more realistic. The program is fully functional as far as I can tell, though there is the possibility of some bugs slipping through the cracks

import random
import time


class Accounts:
    # Defining Account instance variables.
    def __init__(self, pin, balance, annualInterestRate=3.4):
        self.pin = pin
        self.balance = balance
        self.annualInterestRate = annualInterestRate

    # Class function to return the monthly interest rate.
    def getMonthlyInterestRate(self):
        return self.annualInterestRate / 12

    # class function to calculate difference between the balance and the amount withdrawn.
    def withdraw(self, amount):
        self.balance -= amount

    # class function to calculate the sum between the balance and the amount deposited.
    def deposit(self, amount):
        self.balance += amount

    # Class function to calculate the product of the balance and the annual interest rate.
    def getAnnualInterest(self):
        return self.balance * self.annualInterestRate

    # Class function to calculate the product of the balance and the monthly interest rate.
    def getMonthlyInterest(self):
        return self.balance * self.getMonthlyInterestRate()


# Revieves pin from user input and validates input.
def getAccountPin():
    while True:
        pin = input("\nEnter four digit account pin: ")
        try:
            pin = int(pin)
            if pin >= 1000 and pin <= 9999:
                return pin
            else:
                print(f"\n{pin} is not a valid pin... Try again")
        except ValueError:
            print(f"\n{pin} is not a vaild pin... Try again")


# Recieves user input for option selection and validates selection.
def getSelection():
    while True:
        selection = input("\nEnter your selection: ")
        try:
            selection = int(selection)
            if selection >= 1 and selection <= 4:
                return selection
            else:
                print(f"{selection} is not a valid choice... Try again")
        except ValueError:
            print(f"{selection} is not a valid choice... Try again")


# Returns the current working accounts balance.
def viewBalance(workingAccount):
    return workingAccount.balance


# Recieves user input and validates if input is either yes, y, no, or n.
def correctAmount(amount):
    while True:
        answer = input(f"Is ${amount} the correct ammount, Yes or No? ")
        try:
            answer = answer.lower()
            if answer == "y" or answer == "yes":
                return True
            elif answer == "n" or answer == "no":
                return False
            else:
                print("Please enter a valid response")
        except AttributeError:
            print("Please enter a valid response")


# Recieves user input on amount to withdraw and validates inputed value.
def withdraw(workingAccount):
    while True:
        try:
            amount = float(input("\nEnter amount you want to withdraw: "))
            try:
                amount = round(amount, 2)
                if amount > 0 and ((workingAccount.balance) - amount) > 0:
                    answer = correctAmount(amount)
                    if answer == True:
                        print("Verifying withdraw")
                        time.sleep(random.randint(1, 2))
                        return amount
                elif (((workingAccount.balance) - amount) < 0):
                    print("\nYour balance is less than the withdraw amount")
                elif amount == 0:
                    answer = correctAmount(amount)
                    if answer == True:
                        print("Canceling withdraw")
                        time.sleep(random.randint(1, 2))
                        return amount
                else:
                    print("\nPlease enter an amount greater than or equal to 0")
            except TypeError:
                print("\nAmount entered is invalid... Try again")
        except ValueError:
            print("\nAmount entered is invalid... Try again")


# Recieves user input on amount to deposit and validates inputed value.
def deposit(workingAccount):
    while True:
        try:
            amount = float(input("\nEnter amount you want to deposit: "))
            try:
                amount = round(amount, 2)
                if amount > 0:
                    answer = correctAmount(amount)
                    if answer == True:
                        print("Verifying deposit")
                        time.sleep(random.randint(1, 2))
                        return amount
                elif amount == 0:
                    answer = correctAmount(amount)
                    if answer == True:
                        print("Canceling deposit")
                        time.sleep(random.randint(1, 2))
                        return amount
                else:
                    print("\nPlease enter an amount greater than or equal to 0")
            except TypeError:
                print("\nAmount entered is invalid... Try again")
        except ValueError:
            print("\nAmount entered is invalid... Try again")


# End of program to print out account information and return false to end main loop
def exitATM(workingAccount):
    print("\nTransaction is now complete.")
    print("Transaction number: ", random.randint(10000, 1000000))
    print("Current Interest Rate: ", workingAccount.annualInterestRate)
    print("Monthly Interest Rate: ", workingAccount.annualInterestRate / 12)
    print("Thanks for using this ATM")
    return False


def main():
    # Creating all accounts possible, could be stored or read from a file/database instead for better functionality overall.
    accounts = []
    for i in range(1000, 9999):
        account = Accounts(i, 0)
        accounts.append(account)

    # ATM Processes loop
    loop = True
    while loop == True:
        pin = getAccountPin()
        print(pin)
        # Account session loop
        while loop == True:
            # Menu Selection
            print("\n1 - View Balance \t 2 - Withdraw \t 3 - Deposit \t 4 - Exit ")

            selection = getSelection()

            # Getting working account object by comparing pins
            for acc in accounts:
                # Comparing user inputted pin to pins created
                if acc.pin == pin:
                    workingAccount = acc
                    break

            # View Balance
            if selection == 1:
                print(f"\nYour balance is ${viewBalance(workingAccount)}")
            # Withdraw
            elif selection == 2:
                workingAccount.withdraw(withdraw(workingAccount))
                print(f"\nUpdated Balance: ${workingAccount.balance}")
            # Deposit
            elif selection == 3:
                workingAccount.deposit(deposit(workingAccount))
                print(f"\nUpdated Balance: ${workingAccount.balance}")
            # Exit
            elif selection == 4:
                loop = exitATM(workingAccount)
            # Invalid input
            else:
                print("Enter a valid choice")


if __name__ == "__main__":
    main()
\$\endgroup\$
2
  • 4
    \$\begingroup\$ Beware: monthly interest rate is NOT 1/12th of the annual interest rate \$\endgroup\$ – YSC Sep 10 '20 at 16:35
  • 3
    \$\begingroup\$ So I can't have 0805 as my pin? \$\endgroup\$ – Kevin Sep 10 '20 at 21:00
11
\$\begingroup\$

Nice implementation, few suggestions:

  • it's not realistic to use the pin as the account id. To make it more realistic you could ask for the account id first and then for the pin. Entering the account id would be the simulation of "inserting the card in the ATM".

  • The function getAccountPin() requests input from the user, a better name would be requestAccountPin()

  • The function viewBalance below could be a method of Accounts instead of a global function:

    def viewBalance(workingAccount):
        # Returns the current working accounts balance
        return workingAccount.balance
    
  • To simplify the function withdraw(workingAccount) move the checks on the balance directly in Accounts.withdraw. For example:

    def withdraw(self, amount):
        if amount > 0 and self.balance - amount >= 0:
            self.balance -= amount
            return True
        return False
    
  • Same for deposit(workingAccount), it can be simplified by moving some of the logic into Accounts.deposit:

    def deposit(self, amount):
        if amount > 0:
            self.balance += amount
            return True
        return False
    
  • The class Accounts contains the information of a single account, so you can just call it Account

\$\endgroup\$
10
\$\begingroup\$

Your code is nicely structured into short well-named functions, that's great to see. Here's a few points to improve:

  • check PEP8 convention about variables naming. Function and variable names should follow snake_case, so instead of def viewBalance(workingAccount): it's better to use def view_balance(working_account):

  • try-except blocks should be wrapping relevant code as tightly as possible. If you will wrap your whole code into one big try-except, then technically no error will happen, but sometime you can catch an exception you don't intend to catch, and it can be handled improperly. For example in withdraw function you have two wide nested try-except blocks with the same message. They can be merged together, and they can (should) wrap only the relevant lines. The nice side-effect is that the other code will be less indented, which can improve code readability.

  • there's also a bug in there. In first condition you are checking if the withdrawn amount is greater than zero, but it should be greater or equal instead.

  • you're calling correctAmount() only in case the amount is acceptable or zero, but it should be called even if the balance is incorrect (which can happen more likely exactly because of incorrectly entered amount). And in such case instead of repeating it three times, you can call it only once before doing the branching logic.

def withdraw(working_account):
    while True:
        # try-except block should be wrapping the relevant code as tightly as possible
        try:
            amount = float(input("\nEnter amount you want to withdraw: "))
            amount = round(amount, 2)
        except (ValueError, TypeError):
            print("\nAmount entered is invalid... Try again")
            continue

        # dont repeat `correct_amount` in multiple places
        if not correct_amount(amount):
            continue

        # the second condition should be >= instead of >
        if amount > 0 and (working_account.balance - amount) >= 0:
            print("Verifying withdraw")
            time.sleep(random.randint(1, 2))
            return amount

        elif (working_account.balance - amount) < 0:
            print("\nYour balance is less than the withdraw amount")
        elif amount == 0:
            print("Canceling withdraw")
            time.sleep(random.randint(1, 2))
            return amount
        else:
            print("\nPlease enter an amount greater than or equal to 0")
  • the following are just small gotchas you'd discover over time yourself, but here's a shortcut path: in Python you don't have to compare values explicitly. Everything except 0, None, "", False and empty collections is evaluated to True, so your comparison can be shortened:
while loop == True:
    do_something()
# you can use only `while loop:` instead:
while loop:
    loop = "any value, the condition will still work"
  • similarly, if you need to compare a return value you get from function, but then you don't work with it further, you don't have to assign it to a temporary variable:
answer = correctAmount(amount)
if answer == True:
    print("Verifying withdraw")

# you can write this instead:
if correct_amount(amount):
    print("Verifying withdraw")
  • comparisons can be chained together:
if pin >= 1000 and pin <= 9999:
    return pin
# you can use following:
if 1000 <= pin <= 9999:
    return pin
\$\endgroup\$
6
\$\begingroup\$

I notice a couple of areas that I think could be improved:

  • Generally, Python code uses snake case instead of camel case for formatting variable names. So for example:

    def getMonthlyInterestRate(self):
        return self.annualInterestRate / 12
    

    Would become:

    def get_monthly_interest_rate(self):
        return self.annualInterestRate / 12
    

    But this really isn't super important. As long as you stay consistent with either one, your code will be readable.

  • In getAccountPin, if pin >= 1000 and pin <= 9999: can be simplified too 1000 <= pin <= 9999. This can also be done for your other in range conditionals (e.x: selection >= 1 and selection <= 4 to if 1 <= selection <= 4:).

  • I'm not sure why viewBalance needs to exist? Just get the working account's balance directly using .balance, no need for a getter function here. In general, it's considered better practice to avoid using getters when possible.

  • In deposit and withdraw, you don't need nested try/except blocks. except can take one, or more, errors to intercept: In your caseexcept (ValueError, TypeError). This will make your code much cleaner.

  • I think deposit and withdraw should be methods of Accounts objects, not standalone methods. If Accounts represents bank accounts, it make sense to associate the action of withdrawing and depositing money with the bank accounts.

  • deposit never uses its argument workingAccount.

  • Avoid using the if var == True. It's much simpler and cleaner to just do if var to test whether or not var is True.

\$\endgroup\$
6
\$\begingroup\$

You're on a good start and already have several useful comments in other reviews.

A lot of the code is concerned with the details of getting input from the user -- and it is both tedious and repetitive. I would encourage you to think about how one might generalize the user input process: display message; get input; convert the reply to a meaningful value; validate the value; print a message and/or return a value. Here's a rough sketch of the idea:

def getAccountPin():
    return get_user_input(
        message = 'Enter four digit account pin',
        converter = int,
        validator = (lambda x: x in range(1000, 10000)),
    )

def getSelection():
    return get_user_input(
        message = 'Enter your selection',
        converter = int,
        validator = (lambda x: x in range(1, 5)),
    )

def get_user_input(message, converter, validator):
    while True:
        reply = input('\n' + message + ': ')
        try:
            value = converter(reply)
        except Exception:
            print('Invalid entry. Try again')
        if validator(value):
            return value
        else:
            print('Bad value. Try again')

Some of your current functions fit that approach, but others present some interesting (and probably solvable) challenges. Good luck!

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