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
-
1\$\begingroup\$ What do you mean by "semi right way"? \$\endgroup\$301_Moved_Permanently– 301_Moved_Permanently2016年07月21日 07:35:15 +00:00Commented Jul 21, 2016 at 7:35
-
\$\begingroup\$ @MathiasEttinger I mean I have absolutely no idea what I'm doing lol. \$\endgroup\$YoYoYo I'm Awesome– YoYoYo I'm Awesome2016年07月22日 15:01:48 +00:00Commented Jul 22, 2016 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– radarbob2016年07月23日 02:17:28 +00:00Commented Jul 23, 2016 at 2:17
1 Answer 1
The good
I will start of saying that on the surface it looks like you are writing good code.
- Since most of the functions serve a single purpose you are following the single responsibility principle.
- You have documented your code
- For the most part you follow PEP8 style guide.
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()
anddeposit()
in theBank()
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()
-
\$\begingroup\$ I think your functions
prompt
andprompt_range
contain an infinite loop that you never break out of, because of indentation errors. It is correct inget_user_input
. \$\endgroup\$Graipher– Graipher2016年07月25日 14:25:49 +00:00Commented Jul 25, 2016 at 14:25 -
\$\begingroup\$ Love this answer. Thank you for the great info \$\endgroup\$YoYoYo I'm Awesome– YoYoYo I'm Awesome2016年07月25日 15:52:54 +00:00Commented Jul 25, 2016 at 15:52
-
1\$\begingroup\$ I'm gonna go ahead and give you the bounty, thanks for a great answer! \$\endgroup\$YoYoYo I'm Awesome– YoYoYo I'm Awesome2016年07月27日 15:08:06 +00:00Commented Jul 27, 2016 at 15:08