Removing Duplication
There are several places in your program where you repeat the same logic for prompting the user to enter an int, and then ask them to retry if they type something invalid, this is a good opportunity to create a general function to handle that:
def read_int(prompt_msg, error_msg):
while True:
try:
return int(input(prompt_msg))
except ValueError:
print(error_msg)
Which you could use in several places to eliminate the while True / try / except / break logic which occurs in several places, for example to simplify the deposit_money method:
def deposit_money (self):
deposit = read_int("How much money would you like to deposit?", "This value must be an integer.")
Even without the duplication of this logic, splitting the code to read an int and retry away into a separate function significantly reduces the complexity of the deposit_money method and makes the intent clearer
Separation of Concerns
Your withdraw_money method changes the balance available combines together the code for re-asking the user if they try to go overdrawn as well as changing the account balance - ideally a function should "do one thing, and do it well", so there's an opportunity to split the check-and-re-ask code into another function. For example:
def read_withdraw_amount(self):
while True:
withdraw = read_int("How much money would you like to withdraw?", "This value must be an integer.")
if withdraw > self.balance:
print("Sorry, you do not have enough money in your account.")
else:
return withdraw
def withdraw_money(self):
withdraw = self.read_withdraw_amount()
print("You have successfully withdrawn £{}.".format(withdraw))
self.balance -= withdraw
self.display_balance()
Object Oriented Programming
I have created a simple banking system in order to practice OOP (which I am still finding a little confusing).
It's not at all un-common to find OOP confusing as it's a subjective art at the best of times; I would try to approach it with the mindset of keeping the two core principles in-mind of High Cohesion and Loose Coupling, which to summarise in plain English is roughly this:
- High Cohesion is about grouping together functions/methods and state/data which are conceptually very closely related to each other - for example, your
deposit and withdraw methods are both very closely related to the concept of the balance of a single bank account since they both affect the balance of one account at a time.
- Loose Coupling is about minimising the extent to which functions/methods and classes need to depend upon each other. For example, your
Bank class contains a list of all accounts, yet the deposit and withdraw methods only affect a single account at any one time, so conceptually there's no need for those methods to have any link to a class which manages a whole list of accounts.
Could someone also shed some light as to whether an Abstract Base Class is needed with this code (another concept I am struggling a little to understand)?
Inheritance and Abstract Base Classes are among many different possible ways of allowing code reuse - so a typical reason to use it would be having multiple classes which would otherwise require copies of similar or the same methods/behaviour; an abstract base class might be a suitable tool for sharing/reusing that code between those classes,
However, code reuse in this way should only happen where there's a very strong conceptual "identity" link between those classes. Inheritance implies tight coupling, so the relationship between a derived class and a base class should be that the derived class is a specialisation of that base class (i.e. the code in a derived class specialises or extends whatever exists in the base class).
There are no hard-and-fast rules about whether or not to use inheritance, since it really all links back to whatever problem/concept/real-world-thing you're trying to model using classes in the first place.
However, base classes aren't needed in any kind of strict sense; there are often better ways of writing Object-Oriented code without using inheritance or base classes at all. Inheritance implies tight coupling between a derived class and a base class, so it's something which should be used with caution, and often avoided.
There are other ways to reuse code, including Composition, which tends to provide looser coupling, and still provides all the same opportunities for code reuse. Composition allows a class to hold a reference to an object of another class without being so tightly-coupled to it.
Introducing a SavingsAccount class
One way which you could improve the Coupling and Cohesion of your program could be to introduce a SavingsAccount class which is decoupled from the Bank class, responsible for all methods whose purpose is only to access or modify a single SavingsAccount at any one time.
For example:
class SavingsAccount:
def __init__(self, balance):
# the Account Number and Balance are now named class variables
self.account_no = ''.join(["{}".format(randint(0, 9)) for num in range(0, 5)])
self.balance = balance
def print_account_no(self):
print("Your account number is {}.".format(self.account_no))
def display_balance (self):
print("You currently have £{} in your account.".format(self.balance))
def read_withdraw_amount(self):
while True:
withdraw = read_int("How much money would you like to withdraw?", "This value must be an integer.")
if withdraw > self.balance:
print("Sorry, you do not have enough money in your account.")
else:
return withdraw
def withdraw_money(self):
withdraw = self.read_withdraw_amount()
print("You have successfully withdrawn £{}.".format(withdraw))
self.balance -= withdraw
self.display_balance()
def deposit_money (self):
deposit = read_int("How much money would you like to deposit?", "This value must be an integer.")
print("You have successfully deposited £{}.".format(deposit))
self.balance += deposit
self.display_balance()
Just a note on the __init__ method:
def __init__(self, balance):
self.account_no = ''.join(["{}".format(randint(0, 9)) for num in range(0, 5)])
self.balance = balance
When a SavingsAccount object is created, this method will run, so the code which creates the object will need to provide the initial balance. like this:
# Create a SavingsAccount object with £100 starting balance
account = SavingsAccount(100)
There's a few benefits here:
- the
account_no and balance for a SavingsAccount have a strong name which makes their intent much more obvious. Previously those were "magic" numbers of [1] and [0], so it had readability issues in needing the person reading the code to realise/remember the difference between 0 and 1
- Provides clean logical separation between the
Bank and the SavingsAccount - the SavingsAccount class has no connections to the Bank at all, it's entirely self-contained.
- Methods such as
deposit_money and withdraw_money are less "noisy" since they no longer need to pluck the account from a list belonging to the Bank.
With that in mind, the Bank class gets a whole lot smaller, and is no longer concerned with code whose responsibility is around the state of individual accounts in the list. The Bank knows how to create a new account, and how to provide access to an existing account:
class Bank:
def __init__ (self):
self.savingsAccount: Dict[str, SavingsAccount] = {}
def create_account(self):
name = input("Please input your full name: ")
deposit = read_int("Please input the amount of your initial deposit: ", "Please input a valid integer amount. ")
print("You have deposited £{}".format(deposit))
account = SavingsAccount(deposit)
self.savingsAccount[name] = account
account.print_account_no()
def access_account(self):
while True:
name = input("Please input your full name: ")
if name in self.savingsAccount.keys():
account = self.savingsAccount[name]
while True:
account_no = input("Please enter your account number: ")
if account.account_no == account_no:
break
else:
print("There is no such account number associated with this name.")
break
else:
print("We cannot find this name in our system.")
return account
The relationship between Bank and SavingsAccount is one of Composition -- i.e. the Bank owns zero-or-more SavingsAccount objects.
Note that any code which needs to access or modify the fields (variables) within a SavingsAccount object, such as printing the account number, is now part of that SavingsAccount class too.
Since the account-specific methods are no longer part of Bank, the code which needs to work with those methods will need a SavingsAccount object, so the access_account method can return the SavingsAccount object from the list.
Back to possible uses of inheritance: Perhaps if you decided to extend the program in future, there might be different variations on the SavingsAccount which the bank could own - e.g. maybe you would create a PensionAccount class which disallows someone from withdrawing any money since it's a deposit-only account -- in that case, it could indeed make sense to treat the SavingsAccount as a base class and inherit the PensionAccount from it then override the withdraw method.
The menu code further down which handles Account-specific concerns such as letting the user display/deposit/withdraw are only concerned with a SavingsAccount object, which leads onto the next item:
Menu
- There's a lot of duplication involving
previous_page(), break, and continue which could be eliminated by re-structuring the logic slightly.
- There's no need for separate
userchoice1 and userchoice2 variables - one variable is enough.
- The names of the
user_choice1() and user_choice2() functions aren't very descriptive - try to use verbs with function names. The functions are for menu options related to either the Bank or the Account, so something like do_bank_menu() and do_account_menu() would be more insightful for someone reading the code.
The account portion of the menu, now only concerned with accounts and not banks, would need a SavingsAccount object, which it can get from the Bank object:
elif user_choice == "2":
account = bank.access_account()
while True:
user_choice = do_account_menu()
if user_choice == "1":
account.display_balance()
elif user_choice == "2":
account.withdraw_money()
elif user_choice == "3":
account.deposit_money()
elif user_choice == "4":
break
else:
continue
if not previous_page():
break
Menu as a class
Classes aren't just for combining methods and data - classes are often good tools for grouping methods together without any data - all of the menu functions could be grouped together inside a Menu class. This allows the menu itself to be treated as a self-contained unit, and helps create further logical separation between different areas of your program.
Main
It's generally recommended to use a main() function in Python for some of the reasons explained here: https://stackoverflow.com/questions/4041238/why-use-def-main
Putting it Together
Here's a code listing with those structural changes - https://repl.it/repls/FlawedJuniorShareware
from typing import Dict
from random import randint
def read_int(prompt_msg, error_msg):
while True:
try:
return int(input(prompt_msg))
except ValueError:
print(error_msg)
class SavingsAccount:
def __init__(self, balance):
self.account_no = ''.join(["{}".format(randint(0, 9)) for num in range(0, 5)])
self.balance = balance
def print_account_no(self):
print("Your account number is {}.".format(self.account_no))
def display_balance (self):
print("You currently have £{} in your account.".format(self.balance))
def read_withdraw_amount(self):
while True:
withdraw = read_int("How much money would you like to withdraw?", "This value must be an integer.")
if withdraw > self.balance:
print("Sorry, you do not have enough money in your account.")
else:
return withdraw
def withdraw_money(self):
withdraw = self.read_withdraw_amount()
print("You have successfully withdrawn £{}.".format(withdraw))
self.balance -= withdraw
self.display_balance()
def deposit_money (self):
deposit = read_int("How much money would you like to deposit?", "This value must be an integer.")
print("You have successfully deposited £{}.".format(deposit))
self.balance += deposit
self.display_balance()
class Bank:
def __init__ (self):
self.savingsAccount: Dict[str, SavingsAccount] = {}
def create_account(self):
name = input("Please input your full name: ")
deposit = read_int("Please input the amount of your initial deposit: ", "Please input a valid integer amount. ")
print("You have deposited £{}".format(deposit))
account = SavingsAccount(deposit)
self.savingsAccount[name] = account
account.print_account_no()
def access_account(self):
while True:
name = input("Please input your full name: ")
if name in self.savingsAccount.keys():
account = self.savingsAccount[name]
while True:
account_no = input("Please enter your account number: ")
if account.account_no == account_no:
break
else:
print("There is no such account number associated with this name.")
break
else:
print("We cannot find this name in our system.")
return account
class BankMenu:
def __init__(self, bank: Bank):
self.bank = bank
def do_bank_menu(self):
while True:
choice = input("Enter 1 to create an account.\n"
"Enter 2 to access an existing account.\n"
"Enter 3 to exit.\n")
if choice not in ["1", "2", "3"]:
print ("Please enter 1, 2 or 3..")
else:
return choice
def do_account_menu(self):
while True:
choice = input("Enter 1 to display balance.\n"
"Enter 2 to withdraw money.\n"
"Enter 3 to deposit money.\n"
"Enter 4 to return to the main menu.\n")
if choice not in ["1", "2", "3", "4"]:
print ("Please enter 1, 2, 3 or 4.")
else:
break
return choice
def previous_page(self):
while True:
return input("Would you like to return to the previous page? Enter yes or no:")[0].lower() == 'y'
def run(self):
print("Welcome to the bank!")
while True:
user_choice = self.do_bank_menu()
if user_choice == "1":
self.bank.create_account()
if not self.previous_page():
break
elif user_choice == "2":
account = self.bank.access_account()
while True:
user_choice = self.do_account_menu()
if user_choice == "1":
account.display_balance()
elif user_choice == "2":
account.withdraw_money()
elif user_choice == "3":
account.deposit_money()
elif user_choice == "4":
break
else:
continue
if not self.previous_page():
break
else:
break
def main():
print("Welcome to the bank!")
bank = Bank()
bank_menu = BankMenu(bank)
bank_menu.run()
print("Thankyou for using the bank!")
if __name__ == "__main__":
main()
