I have been learning C++ and recently I have started practicing classes. I made this simulation of a banking system with an Account class. I would like to hear feedback on my code, what is good - what I should keep doing, and especially what I should pay more attention to, or if you have any suggestion on how to simplify parts of the code, make it more readable because I feel this could be improved. Also, I would like to hear comments on exception handling, since I am quite new to that as well. Thank you all in advance, I will do my best to implement any of your suggestions.
Account.h
#ifndef ACCOUNT.H
#define ACCOUNT.H
#include <string>
#include <vector>
class Account{
std::string name;
int id;
double balance;
public:
Account();
std::string getName() const;
int getId() const;
double getBalance() const;
void setName(std::string);
void setID(int);
void setBalance(double);
void addAccount (Account);
void withdraw(double);
void deposit(double);
static std::vector<Account> accountDatabase;
};
#endif // ACCOUNT
Account.cpp
#include "Account.h"
#include <iostream>
#include <string>
#include <vector>
Account::Account(){
name = "";
id = 0;
balance = 0;
}
std::vector<Account> Account::accountDatabase;
void Account::addAccount(Account account){
accountDatabase.push_back(account);
}
std::string Account::getName() const{
return name;
}
int Account::getId() const{
return id;
}
double Account::getBalance() const{
return balance;
}
void Account::setName(std::string userName){
name = userName;
}
void Account::setID(int newId){
if (newId < 1)
throw "\n\t\t\t\t ~ ID cannot be zero or negative ~";
for (int i = 0; i < accountDatabase.size(); i++)
if (newId == accountDatabase[i].getId())
throw "\n\t\t\t\t~ Entered ID is already in use ~";
id = newId;
}
void Account::setBalance(double newBalance){
if (newBalance < 0)
throw "\n\t\t\t\t ~ Balance cannot be negative ~";
balance = newBalance;
}
void Account::withdraw(double amount){
if (amount < 0)
throw "\n\t\t\t\t ~ Withdrawal amount cannot be negative ~";
balance -= amount;
}
void Account::deposit(double amount){
if (amount < 0)
throw "\n\t\t\t\t ~ Amount for deposit cannot be negative ~";
balance += amount;
}
Main
#include <iostream>
#include "Account.h"
#include <string>
void printMenu(){
std::cout << "\n" << R"(
Please select one of the following options:
1. Create an account
2. Check balance
3. Withdraw
4. Deposit
5. Account summary
6. Make a transaction
7. Exit
)" << "\n\t\t\t\t--> ";
}
// get a valid input
template<typename Type>
void getInput(Type &value){
while (true){
std::cin >> value;
if (std::cin.fail()){
std::cin.clear();
std::cin.ignore(100, '\n');
std::cout << "\n\t\t\t\t\t~ Invalid input ~"
<< "\n\t\t\t\t--> Enter again: ";
}
else {
std::cin.ignore();
return;
}
}
}
// find account in account database and return index of that account
int findAccount (int id){
for (int i = 0; i < Account::accountDatabase.size(); i++)
if (id == Account::accountDatabase[i].getId()) return i;
return -1;
}
void createAccount (){
Account newAccount;
std::cout << "\n\t\t\t\t--> Please enter your name: ";
std::string name;
std::cin.ignore();
std::getline(std::cin, name);
newAccount.setName(name);
std::cout << "\n\t\t\t\t--> Please enter your ID: ";
int id;
getInput(id);
newAccount.setID(id);
std::cout << "\n\t\t\t\t--> Please enter your balance: ";
double balance;
getInput(balance);
newAccount.setBalance(balance);
// add account to the database
newAccount.addAccount(newAccount);
std::cout << "\n\t\t\t\t~ Your account has been successfully created ~\n";
}
void MenuSelection(){
int option = 1, account, id;
while (option != 7){
try{
switch (option){
case 1: createAccount();break;
// check balance
case 2:{
std::cout << "\n\t\t\t\t--> Please enter your ID: ";
getInput(id);
account = findAccount(id);
if (account == -1) std::cout << "\n\t\t\t\t~ ID does not match any in the database ~\n";
else std::cout << "\n\t\t\t\t--> Your balance: " << Account::accountDatabase[account].getBalance();
} break;
// withdraw money
case 3:{
std::cout << "\n\t\t\t\t--> Enter amount to withdraw: ";
double withdrawalAmount;
getInput(withdrawalAmount);
std::cout << "\n\t\t\t\t--> Please enter your ID: ";
getInput(id);
account = findAccount(id);
if (account == -1) std::cout << "\n\t\t\t\t~ ID does not match any in the database ~\n";
else {
if (Account::accountDatabase[account].getBalance() >= withdrawalAmount){
Account::accountDatabase[account].withdraw(withdrawalAmount);
std::cout << "\n\t\t\t\t~ Amount successfully withdrawn ~\n";
}
else std::cout << "\n\t\t\t~ Withdrawal not successful - check the state of balance ~\n";
}
} break;
// deposit money
case 4:{
std::cout << "\n\t\t\t\t--> Enter amount to deposit: ";
double depositAmount;
getInput(depositAmount);
std::cout << "\n\t\t\t\t--> Please enter your ID: ";
getInput(id);
account = findAccount(id);
if (account == -1) std::cout << "\n\t\t\t\t~ ID does not match any in the database ~\n";
else {
Account::accountDatabase[account].deposit(depositAmount);
std::cout << "\n\t\t\t\t-->~ Amount successfully deposited ~\n";
}
} break;
// print account summary
case 5:{
std::cout << "\n\t\t\t\t--> Please enter your ID: ";
getInput(id);
account = findAccount(id);
if (account == -1) std::cout << "\n\t\t\t\t~ ID does not match any in the database ~\n";
else std::cout << "\n\n\t\t\t\t~ ACCOUNT SUMMARY ~\n"
<< "\n\t\t\t\t--> Name: " << Account::accountDatabase[account].getName()
<< "\n\t\t\t\t--> ID: " << Account::accountDatabase[account].getId()
<< "\n\t\t\t\t--> Balance: " << Account::accountDatabase[account].getBalance() << "\n";
} break;
// make a transaction
case 6:{
std::cout << "\n\t\t\t\t--> Do you wish to withdraw or deposit? (w/d): ";
char choice;
std::cin >> choice;
std::cout << "\n\t\t\t\t--> Enter transaction amount: ";
double amount;
getInput(amount);
std::cout << "\n\t\t\t\t--> Please enter your ID: ";
getInput(id);
account = findAccount(id);
if (account == -1) std::cout << "\n\t\t\t\t~ ID does not match any in the database ~\n";
else {
std::cout << "\n\t\t\t\t--> Please enter ID of the transaction account: ";
getInput(id);
int secondAccount = findAccount(id);
if (secondAccount == -1) std::cout << "\n\t\t\t\t~ ID does not match any in the database ~\n";
else {
bool transactionPerformed = false;
if (tolower(choice) == 'w' && Account::accountDatabase[secondAccount].getBalance() >= amount){
Account::accountDatabase[secondAccount].withdraw(amount);
Account::accountDatabase[account].deposit(amount);
transactionPerformed = true;
}
else if (tolower(choice) == 'd' && Account::accountDatabase[account].getBalance() >= amount){
Account::accountDatabase[secondAccount].deposit(amount);
Account::accountDatabase[account].withdraw(amount);
transactionPerformed = true;
}
if (!transactionPerformed) std::cout << "\n\t\t\t\t~ Transaction not successful ~\n";
else std::cout << "\n\t\t\t\t~ Transaction successfully completed ~\n";
}
}
}
}
}
catch (const char* msg){
std::cerr << msg;
}
printMenu();
getInput(option);
while (option < 1 || option > 7){
std::cout << "\n\t\t\t\t--> Please enter a valid option (1-7): ";
getInput(option);
}
}
std::cout << R"(
########################################################################################################################
~ THANK YOU FOR USING OUR SERVICES ~
########################################################################################################################
)";
}
int main(){
std::cout << R"(
########################################################################################################################
~ W E L C O M E T O O U R B A N K ~
########################################################################################################################
)";
MenuSelection();
return 0;
}
2 Answers 2
I have a few suggestions:
\n\t\t\t\t
is everywhere, move it into a function which takes and returns a string, and prepends whatever string it is given with this format - the benefit of this is that if you change your formatting in the future, you only need to do it in one place.- Getting the user to enter their ID is a common operation, so put this logic all in one place. i.e.
Should be lopped out into its own function, again for the same reason - if the way you want to do it changes, you only need to do it in one place!std::cout << "\n\t\t\t\t--> Please enter your ID: "; getInput(id); account = findAccount(id);
- Move the code within each case block into its own function, e.g for
case 2
, just make a function calledcheckBalance
which does exactly that. Call it from the case block (similar to how you have done forcase 1
). The comments already hint at what each block does, but the whole of the switch/case statement is quite a lot of code! - The way you look up accounts is a little confusing and potentially inefficient for large numbers of accounts, I would use a map (http://www.cplusplus.com/reference/map/map/) with the key being the ID.
I hope this is enough to get you on your way, if you give this another stab then I'm happy to take another look!
The class is doing too much: it is not the task of the
Account
class to also keep a database of all accounts. Remember: one class (or function), one responsibility. By the same principle,menuSelection
does too much in your main program.Use the initializer list in your constructor, i.e., do
Account::Account() : name_(), id_(0), balance(0) {}
. But even better, you should always try to avoid defining default operations, as per C.20 of the C++ Core Guidelines.Pass complex (i.e., not built-in types) by const-ref. This includes strings and Account types. For example, rather do
void Account::setName(const std::string& userName) { ... }
. Another possibility - if your compiler is recent enough - is to use std::string_view.Avoid throwing character strings and throw proper objects instead, see E.14.
You seem to have an invariant which says that the balance can never be negative. I would enforce this more consistently by writing a private function like
void checkInvariant() { ... }
that makes things likeassert(balance > 0)
(but be careful when comparing floating point values to constant values; it should be done with a threshold). Then, you can add this check to suitable functions and the invariant is nicely enforced by the class itself.The above can help you catch errors like in
withdraw
: is it acceptable that the amount becomes negative here?
Explore related questions
See similar questions with these tags.