6
\$\begingroup\$

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;
}
\$\endgroup\$

2 Answers 2

6
\$\begingroup\$

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.
    std::cout << "\n\t\t\t\t--> Please enter your ID: ";
    getInput(id);
    account = findAccount(id);
    
    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!
  • Move the code within each case block into its own function, e.g for case 2, just make a function called checkBalance which does exactly that. Call it from the case block (similar to how you have done for case 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!

\$\endgroup\$
5
\$\begingroup\$
  • 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 like assert(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?

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.