1
\$\begingroup\$

I have created a gorgeous ATM machine program. It makes all other ATMs look like crap, if I do say so myself. What I would like is some sort of critique on what I've done:

  • Is there better syntax I could use for the if/else's such as ternarys?
  • Can I improve this in any way?
  • Can I move more of the code into separate methods?

Source:

from random import randint


# The amount of money that will be processed each time
# taken from randint()
CHECKING_MONEY = randint(0, 1000)
SAVINGS_MONEY = randint(0, 1000)


def die(why):

    """Kill the program."""

    print "{} Exiting..".format(why)
    quit(1)


def broke(excuse, money):

    """You're broke!"""

    print "{}. ${} available in account.".format(excuse, money)


def formatter():

    """Shouldn't have to explain this.."""

    return '-' * 50


def prompt(info):

    """Prompt for information"""

    return raw_input(info)


def check_if_true(money):

    """Make sure there's money in the accounts."""

    if money > 0:
        return True
    else:
        return False


def check_account(money):

    """Function to check account balance."""

    print "${} currently available in account".format(money)
    choice = prompt("Main Menu? ").lower()
    if 'yes' in choice:
        welcome_screen()
    else:
        die("Goodbye.")


def transfer_funds(checking, savings):

    """Function to transfer funds back and forth from accounts."""

    total = checking + savings
    check = check_if_true(total)
    # If the check returns true then continue with the function
    if check == True:
        print """
    [ 1 ] Checking account to Savings account
    [ 2 ] Savings account to Checking account
    [ 3 ] Return to Main Menu
        """

        choice = int(prompt("Which account: "))
        print formatter()  # formatting makes everything pretty

        if choice == 1:
            print "Currently ${} in Checking account.".format(checking)
            amount = int(prompt("Enter amount to transfer to Savings account: $"))
            if amount > checking:  # Only allow an amount less than the amount in checking
                print "You do not have that much money."
                transfer_funds(checking, savings)
            else:
                checking -= amount
                savings += amount
                print "${} withdrawn from Checking and added to Savings.".format(amount)
                print "${} left in Checking account.".format(checking)
                print "${} now in Savings account.".format(savings)
                choice = prompt("Transfer more funds? ").lower()
                print formatter()
                if 'no' in choice:
                    die("Goodbye.")
                    print formatter()
                else:
                    transfer_funds(checking, savings)  # recursively loop back to the function

        elif choice == 2:
            print "Currently ${} in Savings account.".format(savings)
            amount = int(prompt("Enter amount to transfer to Checking account: $"))
            if amount > savings:
                print "You do not have that much money."
                print formatter()
                transfer_funds(checking, savings)
            else:
                savings -= amount
                checking += amount
                print "${} withdrawn from Savings and added to Checking.".format(amount)
                print "${} left in Savings account.".format(savings)
                print "${} now in Checking account.".format(checking)
                choice = prompt("Transfer more funds? ").lower()
                print formatter()
                if 'no' in choice:
                    die("Goodbye.")
                    print formatter()
                else:
                    print formatter()
                    transfer_funds(checking, savings)

        elif choice == 3:
            welcome_screen()
        else:
            die("Invalid option.")
    else:
        broke("You do not have enough money.", total)


def withdraw_funds(checking, savings):

    """Function to withdraw funds from the accounts."""

    money_in_checking = check_if_true(checking)
    money_in_savings = check_if_true(savings)

    print """
    [ 1 ] Withdraw from Checking account
    [ 2 ] Withdraw from Savings account
    [ 3 ] Return to Main Menu
    """

    choice = int(prompt("Which account would you like to withdraw from? "))
    print formatter()

    if choice == 1:

        if money_in_checking == True:
            print "${} currently available in Checking account.".format(checking)

            while checking > 0:  # While the accounts balance isn't 0.
                amount = int(prompt("Enter amount to withdraw from Checking account: $"))
                if amount <= checking:
                    checking -= amount
                    print "${} withdrawn from Checking. ${} left.".format(amount, checking)
                    again = prompt("Withdraw more funds? ").lower()
                    print formatter()
                    if 'no' in again:
                        die("Goodbye")
                    else:
                        withdraw_funds(checking, savings)
                else:
                    print "You do not have that much money."
                    print formatter()
                    withdraw_funds(checking, savings)
            else:
                print "Unable to withdraw anymore ${} currently available in account.".format(checking)
        else:
            broke("Unable to withdraw anymore money", checking)
            welcome_screen()  # recursively loop back to the welcome screen

    elif choice == 2:
        if money_in_savings == True:

            while savings > 0:
                print "${} currently available in Savings account.".format(savings)
                amount = int(prompt("Enter amount to withdraw from Savings account: $"))
                if amount <= savings:
                    savings -= amount
                    print "${} withdrawn from Savings. ${} left.".format(amount, savings)
                    again = prompt("Withdraw more funds? ").lower()
                    print formatter()
                    if 'no' in again:
                        die("Goodbye")
                    else:
                        amount = int(prompt("Enter amount to withdraw from Savings account: $"))
                else:
                    print "You do not have that much money."
                    print formatter()
                    withdraw_funds(checking, savings)
            else:
                print "Unable to withdraw anymore ${} currently available in account.".format(savings)
        else:
            broke("Unable to withdraw anymore money", savings)
            welcome_screen()

    elif choice == 3:
        welcome_screen()
    else:
        die("Invalid option.")


def deposit_funds(checking, savings):

    """Function to deposit funds into the accounts."""

    print """
    [ 1 ] Deposit into Checking account
    [ 2 ] Deposit into savings account
    [ 3 ] Return to Main Menu
    """

    choice = int(prompt("Which account: "))
    print formatter()

    if choice == 1:
        amount = int(prompt("Enter amount to deposit into Checking: "))
        checking += amount
        print "${} now available in Checking account.".format(checking)
        more = prompt("Deposit more? ")
        if 'no' in more:  # If you say no it will exit.
            die("Goodbye")
        else:
            print formatter()
            deposit_funds(checking, savings)
    elif choice == 2:
        amount = int(prompt("Enter amount to deposit into Savings: "))
        savings += amount
        print "${} now available in Savings account.".format(savings)
        more = prompt("Deposit more? ")
        if 'no' in more:
            die("Goodbye")
        else:
            print formatter()
            deposit_funds(checking, savings)
    elif choice == 3:
        welcome_screen()
    else:
        die("Invalid choice")


def welcome_screen():

    """Main function of the program"""

    print formatter()
    print """
    Welcome to the Bank!
    Options include:
    [ 1 ] View Checking account
    [ 2 ] View Savings account
    [ 3 ] Transfer funds
    [ 4 ] Withdraw funds
    [ 5 ] Deposit funds
    """

    choice = int(prompt("What would you like to do: "))
    print formatter()
    if choice == 1:
        check_account(CHECKING_MONEY)
    elif choice == 2:
        check_account(SAVINGS_MONEY)
    elif choice == 3:
        transfer_funds(CHECKING_MONEY, SAVINGS_MONEY)
    elif choice == 4:
        withdraw_funds(CHECKING_MONEY, SAVINGS_MONEY)
    elif choice == 5:
        deposit_funds(CHECKING_MONEY, SAVINGS_MONEY)
    else:
        die("Invalid option.")


if __name__ == '__main__':
    welcome_screen()

When this is run: enter image description here

\$\endgroup\$
4
  • 1
    \$\begingroup\$ What do you mean by "semi right way"? \$\endgroup\$ Jul 21 '16 at 7:35
  • \$\begingroup\$ @MathiasEttinger I mean I have absolutely no idea what I'm doing lol. \$\endgroup\$ Jul 22 '16 at 15:01
  • 4
    \$\begingroup\$ It makes all other ATMs look like crap, if I do say so myself. Ah. The Dunning-Kruger Affect. However, you did say "I have .. no idea what I'm doing." This is a most encouraging sign that you will become an excellent programmer. \$\endgroup\$
    – radarbob
    Jul 23 '16 at 2:17
  • \$\begingroup\$ @radarbob Well thank you, I appreciate that! \$\endgroup\$ Jul 25 '16 at 15:31
5
+50
\$\begingroup\$

The good

I will start of saying that on the surface it looks like you are writing good code.


The bad

As I said it looks like you are doing the right things, however as one looks deeper there still are some fundamental flaws. Your codingstyle reminds me of someone blindly filling out a checklist. All the things on the checklist are correct, but it does not quite seem like you understand why. Therefore some things turns out a bit weird.

def die(why):

    """Kill the program."""

    print "{} Exiting..".format(why)
    quit(1)

This is perhaps the clearest example of what I mean. Documentation in programming is a necessary evil. To the best of your abilites you should write code in such a way that the code itself tells what it does. Documentation is for telling why. Looking at your example above the documentation does not add anything to your code.

Using quit() is a common among beginners. Nevertheless, quit should not be used in production code. This is because it only works if the site module is loaded. Instead, this function should only be used in the interpreter. There are several methods of avoiding using quit(). Since a function terminates when it has reached the last line, hence there is no need to explicitly call quit(). A second option is to use The return statement which causes your function to exit and hand back a value to its caller.


The ugly

You expect your users to be rational logical beings, however that is not always the case. When it comes to user input always expect the unexpected. You have the following code several times in your code

choice = int(prompt("What would you like to do: "))

Firstly the function prompt is as it stands useless and confusing. It does nothing more than renaming raw_input() a command which all Python users should be familiar with. The ugly part is that the above code breaks whenever the user inputs a string or anything out of the ordinary.

To handle this it is a good idea to have a separate function handling user input. A simple example could be

def prompt():
    while True:
        choice = raw_input("What would you like to do: ")
        try:
            return int(choice)
        except ValueError:
            print("The input must be an integer")

This can of course be made more fancy to ensure that the user actually inputs a valid choice, and not just some random integer.

def prompt(choices):
    while True:
        choice = raw_input("What would you like to do: ")
        try:
            choice = int(choice)
            if choice in choices:
                return choice
        except ValueError:
            print("The input must be an integer")

This is fine for prompting the user to select the right option, but what if we want to select money? We are then not interested in picking a number for a list, but rather any number in a given range.

def prompt_range(choices):
    low, high = min(choices), max(choices)
    while True:
        choice = raw_input("What would you like to do: ")
        try:
            choice = int(choice)
            if choice >= low and choice <= high:
                return choice
        except ValueError:
            print("The input must be an integer")

In my solution below I have fused these two solutions together. I used the command excact to switch between a range, and exact input.I also used two ternary operators, not because they are needed (the code would be clearer without them), but to show you that they could be used.

def get_user_input(choices, excact=True):
    low, high = min(choices), max(choices)

    def correct_input():
        return (choice in choices) if excact else (choice >= low and choice <= high)

    display = "\n Select any number from "
    display += str(choices) if excact else '{} to {}'.format(low, high)

    print display
    while True:
        choice = raw_input("choice: ")
        try:
            choice = int(choice)
            if correct_input():
                return choice
        except ValueError:
            print("Ops, {} was not an legal option. Please try again!", display)

KISS

A bigger problem with your code as mentioned in the introduction is the use of one-liner functions. There are times where these can be useful, and many will disagree with me. However in your code I really do not see them being useful. Either 1) remove them, or 2) more more code into them. As I did with handling user input.

Hard_coding

Another bigger problem is what happens if I only have one account, or if I have three? You have everything related to the accounts hardcoded, including the names of the accounts. You could solve all of these problems using classes instead.

A third alternative: Classes

Here is a short introduction and by "coincidence" this link uses a bank account to show how useful classes can be. I have used large parts of that link in the question below, and added a few bits if my own.

One solution

Below is one attempt to incorporate the ideas stated above. However it works, and IMHO is a better starting point. Feel free to drop into chat to ask any follow up questions on this method. The things missing from the code below are

  • Proper docstrings. They need to be even more descriptive
  • Ironing out any bugs in the code
  • The withdraw() and deposit() in the Bank() class needs to be written.

These things are either left for you to figure out, or another answer =)

from random import randint

#==========================================================
# ONLY FOR TESTING
#==========================================================

checking_account = Account('Checking', randint(0, 1000))
savings_account = Account('Savings', randint(0, 1000))

test_bank = Bank([checking_account, savings_account])
#==========================================================

OPTIONS = ['Create new account',
           'Transfer funds',
           'Withdraw funds',
           'Deposit funds',
           'Exit']

NUMBER_OF_OPTIONS = len(OPTIONS)


WIDTH = 50


def formatter(width=WIDTH):
    return '\n' + '-' * width


def get_user_input(choices, excact=True):
    low, high = min(choices), max(choices)

    def correct_input():
        return (choice in choices) if excact else (choice >= low and choice <= high)

    display = "\n Select any number from "
    display += str(choices) if excact else '{} to {}'.format(low, high)

    print display
    while True:
        choice = raw_input("choice: ")
        try:
            choice = int(choice)
            if correct_input():
                return choice
        except ValueError:
            print("Ops, {} was not an legal option. Please try again!", display)


def welcome_screen():

    while True:
        print test_bank, '\n'

        for i, name in enumerate(OPTIONS):
            print "[ {} ] {}".format(i+1, name)

        user_choice = get_user_input(range(1, NUMBER_OF_OPTIONS+1))
        if user_choice == 1:
            test_bank.create_account()
        elif user_choice == 2:
            test_bank.transfer_funds()
        elif user_choice == 3:
            test_bank.withdraw_funds()
        elif user_choice == 4:
            test_bank.deposit_funds()
        elif user_choice == 5:
            break
    print "Thank you for using the bank!"


class Bank(object):

    """A customer of ABC Bank with a checking account. Customers have the
    following properties:

    Attributes:
        name: A string representing the customer's name.
        balance: A float tracking the current balance of the customer's account.
    """

    def __init__(self, list_of_accounts=[]):
        """Return a Bank object whose name is *name* and starting
        balance is *balance*."""
        self.accounts = list_of_accounts

    def transfer_funds(self):
        """Transfers funds from one account in your bank to another"""
        if len(self.accounts) < 2:
            raise ValueError(
                'You need to have atleast two accounts to transfer money')

        for i, account in enumerate(self.accounts):
            print "[ {} ] {}".format(i+1, account.name)

        while True:
            print("Write in the account you want to withdraw money from")
            account_numbers = range(1, len(self.accounts) + 1)
            account_A = get_user_input(account_numbers) - 1
            account_numbers.remove(account_A + 1)

            print("Write in the the account you want to deposit money into")
            account_B = get_user_input(account_numbers) - 1

            if account_A != account_B:
                break
            print("You can not withdraw and deposit from the same account")

        balance = self.accounts[account_A].balance
        if balance == 0:
            raise ValueError(
                'You can not withdraw money from an empty account')

        print('Write in the amount of money you want to withdraw')

        amount = get_user_input([0, balance], False)

        self.accounts[account_A].withdraw(amount)
        self.accounts[account_B].deposit(amount)

    def withdraw_funds(self, account_number):
        # Left as an excercise
        return ''

    def diposit_funds(self, account_number):
        # Left as an excercise
        return ''

    def create_account(self):
        name = raw_input("Write in the name of the new account: ")
        balance = raw_input(
            "Write in the amount of money in the {} account: ".format(name))
        while True:
            try:
                if balance > 0:
                    break
            except ValueError:
                pass
            print("Ops, please write an integer number of money")
        self.accounts.append(Account(name, float(balance)))

    def __str__(self):
        lst = []
        for account in self.accounts:
            lst.append(str(account))
        return formatter(width=WIDTH) + ''.join(lst) + formatter(width=WIDTH)


class Account(object):

    """Account have the following attributes:

    Attributes:
        name: A string representing the customer's name.
        balance: A float tracking the current balance of the customer's account.
    """

    def __init__(self, name, balance=0.0):
        """Return a Customer object whose name is *name* and starting
        balance is *balance*."""
        self.name = name
        self.balance = balance

    def withdraw(self, amount):
        """Return the balance remaining after withdrawing *amount*
        dollars."""
        self.balance -= amount
        return self.balance

    def deposit(self, amount):
        """Return the balance remaining after depositing *amount*
        dollars."""
        self.balance += amount
        return self.balance

    def __str__(self):
        """Prints the name of the account and the number of money.
        Pdding for name is 10 characters, while the money has 10 + 2 decimals."""
        return '\n{:>10}: {:10.2f}$'.format(self.name, self.balance)


if __name__ == '__main__':

    welcome_screen()
\$\endgroup\$
3
  • \$\begingroup\$ I think your functions prompt and prompt_range contain an infinite loop that you never break out of, because of indentation errors. It is correct in get_user_input. \$\endgroup\$
    – Graipher
    Jul 25 '16 at 14:25
  • \$\begingroup\$ Love this answer. Thank you for the great info \$\endgroup\$ Jul 25 '16 at 15:52
  • 1
    \$\begingroup\$ I'm gonna go ahead and give you the bounty, thanks for a great answer! \$\endgroup\$ Jul 27 '16 at 15:08

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.