7
\$\begingroup\$

I'm new to C++ and wrote this Bank Management System app. It's one of the projects that everyone does as a beginner. This app runs perfectly so far. I tried to use OOP and encapsulation in this app. It also helps the User to store and retrieve data from HDD.

I'd appreciate if someone could give me feedback in the areas where I could've written better than What I've done.

bankclassic.h

#ifndef BANK_CLASSIC_H
#define BANK_CLASSIC_H

#include <iostream>
#include <string>
#include <vector>
#include <iomanip>
#include <Windows.h>
#include <cstdio>
#include <conio.h>
#include <sstream>
#include <numeric> // for std::iota()
#include <cctype> // for isdigit()
#include <fstream>

namespace BankClassic
{
    struct BankInfo // declaration of bank data type, definition is hidden
    {
        std::string acNum, acName, acType, acBalance;
        BankInfo(std::string accountNumber, std::string accountName, std::string accountType, std::string accountBalance); // normal constructor with 4 pararms
        BankInfo(); // constructs an empty BankInfo
        //list of setters
        void setAcNum(std::string value);
        void setName(std::string value);
        void setType(std::string value);
        void setBalance(std::string value);
    };

    //declearations of all the abstruct types
    typedef std::vector<BankInfo> bank_Vec;
    typedef std::vector<std::string> string_vec;
    typedef std::vector<char> char_vec;
    typedef unsigned char char_u;
    typedef unsigned int int_u;
    typedef std::vector<std::vector<std::string>> vec_str_vec;
    typedef std::vector<int_u> int_u_vec;

    class Display
    {
    public:
        Display();
        ~Display();

        /*
        Displays the main menu
        */
        static char_u displayMainMenu();

        /*
        Displays the cover menu
        */
        static void coverMenu();

        /*
        Helper function for: ---> Functions::displayAccountStatus(std::string acNum)
        */
        static void accountStatus(BankInfo* bankInfo);

        /*
        Helper function for: ---> Functions::displayAccountHolders()
        */
        static void displayAccountHolderList(bank_Vec& accountContainer);
    };

    class Functions
    {
    private:
        bank_Vec accountCollection; // place to hold all the account data in the memory
    public:
        Functions();
        ~Functions();
        static BankInfo getAccountVec(BankInfo* bankInfo);
        static void createNewAccount();
        static void updateAccount(BankInfo* bankInfo, bool withdraw = false);
        static void displayAccountStatus(BankInfo* bankInfo);
        static void displayAccountHolders();
        static void deleteAccount(BankInfo* bankInfo);
        static void modifyAccount(BankInfo* bankInfo);
        static void appExit();
        static BankInfo getAccountNumber();

        /*
        Runs the application
        */
        static void run();
    };

    namespace UtilityTools
    {
        class StringManip
        {
        private:
            enum class _split : bool { all = true, noEmpty = false };
        public:
            typedef _split splitOption;

            /*
            returns a vector of splited word specified by the param 'separator'
            */
            static string_vec split(const char* word, char separator, splitOption split_option);

        };

        class File
        {
        public:
            /*
            Writes all the elements of a bank_Vec to DAT files in the HDD
            */
            static void writeToDatFile(bank_Vec& vecCont);

            /*
            Returns all the elements from DAT files as a bank_Vec
            */
            static bank_Vec readFromDatFile();
        };
    }
}
#endif // !BANK_CLASSIC_H

/*
Written by Alex
*/

BankClassic.cpp

#include "bankclassic.h"

namespace BankClassic
{
    namespace UtilityTools
    {
        string_vec StringManip::split(const char* word, char separator, splitOption split_option)
        {
            std::vector<std::string> splited_data;
            std::stringstream ss(word);
            std::string token;
            while (std::getline(ss, token, separator))
            {
                if (split_option == splitOption::noEmpty) { if (!token.empty()) splited_data.push_back(token); }
                if (split_option == splitOption::all) splited_data.push_back(token);
            }
            return splited_data;
        }

        void File::writeToDatFile(bank_Vec& bankInfoVec)
        {
            std::ofstream ofs("bankinfo.dat");
            for (const BankInfo& data : bankInfoVec) ofs << data.acNum << ", " << data.acName << ", " << data.acType << ", " << data.acBalance << "\n";
        }

        bank_Vec File::readFromDatFile()
        {
            bank_Vec dataVec;
            std::ifstream ifs("bankinfo.dat");
            std::string line;
            while (std::getline(ifs, line))
            {
                std::istringstream iss(line);
                std::string acNum, name, type, balance;
                std::getline(iss, acNum, ',');
                iss.get();
                std::getline(iss, name, ',');
                iss.get();
                std::getline(iss, type, ',');
                iss.get();
                std::getline(iss, balance);
                dataVec.push_back(BankInfo(acNum, name, type, balance));
            }
            return dataVec;
        }
    }

    // definitions of BankInfo struct and its elements

    BankInfo::BankInfo(std::string accountNumber, std::string accountName, std::string accountType, std::string accountBalance)
    {
        acNum = accountNumber;
        acName = accountName;
        acType = accountType;
        acBalance = accountBalance;
    }

    BankInfo::BankInfo()
    {
        acNum = "0";
        acName = "0";
        acType = "0";
        acBalance = "0";
    }

    void BankInfo::setAcNum(std::string value)
    {
        if (value != "") acNum = value;
    }

    void BankInfo::setName(std::string value)
    {
        if (value != "") acName = value;
    }

    void BankInfo::setType(std::string value)
    {
        if (value == "S" || value == "C") acType = value;
    }

    void BankInfo::setBalance(std::string value)
    {
        if (value != "") acBalance = value;
    }


    Display::Display() {}
    Display::~Display() {}

    /*
    Returns a range of integers in a form of an int vector
    */
    int_u_vec range(int_u start, int_u size) // internal helping function
    {
        int_u_vec intVec(size);
        std::iota(std::begin(intVec), std::end(intVec), start);
        return intVec;
    }

    /*
    Checks a string is equal to a number
    =>  "123456" ----> true
    =>  "123ZZ6" ----> false
    */
    bool isDigit(const std::string& s) // internal helping function
    {
         std::string::const_iterator it = s.begin();
        while (it != s.end() && std::isdigit(*it)) ++it; // std::isdigit() from cctype header file
        return !s.empty() && it == s.end();
    }

    /*
    Converts lowercase string to uppercase
    */
    std::string toUpper(std::string x) // internal helping function

    {
        std::stringstream ss;
        const int size = x.size();
        std::vector<char> tempLo(size);
        std::vector<char> tempUp(size);
        std::vector<char> Upper(26);
        for (int i = 0; i < 26; i++) Upper[i] = i + 65;
        for (int i = 0; i < size; i++) tempLo.at(i) = x.at(i);
        for (int i = 0; i < size; i++)
        {
            bool ex = std::find(Upper.begin(), Upper.end(), tempLo.at(i)) != Upper.end();
            if (ex == true) tempUp.at(i) = tempLo.at(i);
            else if (tempLo.at(i) == ' ') tempUp.at(i) = tempLo.at(i);
            else tempUp.at(i) = tempLo[i] - 32;
        }
        for (int i = 0; i < size; i++) ss << tempUp[i];
        return ss.str();
    }

    /*
    Checks if a vector of string vector contains a specific string vector or not
    */
    bool exists(std::vector<std::vector<std::string>>& v1, std::vector<std::string>& v2) // internal helping function (overload 1)
    {
        return std::find(std::begin(v1), std::end(v1), v2) != std::end(v1);
    }

    /*
    Checks a string vector contains a specific string or not
    */
    bool exists(std::vector<std::vector<std::string>>& v, std::string element) // internal helping function (overload 2)
    {
        bool re = false;
        for (int_u i = 0; i < v.size(); i++)
        {
            re = std::find(std::begin(v[i]), std::end(v[i]), element) != std::end(v[i]);
            if (re == true) break;
        }
        return re;
    }

    /*
    Checks if an unsigned int exists in a vector of unsigned int
    */
    bool exists(std::vector<unsigned int> vec, unsigned int element) // internal helping function (overload 3)
    {
        return std::find(std::begin(vec), std::end(vec), element) != std::end(vec);
    }

    /*
    Checks if a char exits in a vector of char*/
    bool exists(std::vector<char>& vec, char element) // internal helping function (overload 4)
    {
        return std::find(std::begin(vec), std::end(vec), element) != std::end(vec);
    }

    /*
    Compares two BankInfo types are equal or not*/
    inline bool operator==(const BankInfo& data1, const BankInfo& data2) // internal operator overload function
    {
        if ((data1.acNum == data2.acNum) && (data1.acName == data2.acName) && (data1.acType == data2.acType) && (data1.acBalance == data2.acBalance)) return true;
        else return false;
    }

    /*
    Compares two BankInfo types are unqual or not
    */
    inline bool operator!=(const BankInfo& data1, const BankInfo& data2) // internal operator overload function
    {
        if ((data1.acNum != data2.acNum) || (data1.acName != data2.acName) || (data1.acType != data2.acType) || (data1.acBalance != data2.acBalance)) return true;
        else return false;
    }

    /*
    Returns the int POS of BankInfo data in a bank_vec
    else---> returns -1 if not found
    */
    int search(bank_Vec& v, BankInfo* bankInfo) // internal helping function
    {
        int re = -1;
        for (int_u i = 0; i < v.size(); i++)
        {
            bool res = v[i].acNum == bankInfo->acNum;
            if (res == true)
            {
                re = i;
                break;
            }
        }
        return re;
    }

    /*
    Checks if a BankInfo type exists in a bank_Vec
    */
    bool exists(bank_Vec& bankInfoVec, BankInfo element) // internal helping function (overload 5)
    {
        return std::find(std::begin(bankInfoVec), std::end(bankInfoVec), element) != std::end(bankInfoVec);
    }

    /*
    Checks if a  std::string type exists in a bank_Vec
    */
    bool exists(bank_Vec& bank, std::string& element) // internal helping function (overload 6)
    {
        bool re = false;
        for (int_u i = 0; i < bank.size(); i++)
        {
            if (bank[i].acNum == element)
            {
                re = true;
                break;
            }
        }
        return re;
    }

    std::string _menuHeadingChar(int size, char character) // internal helping function
    {
        std::string x(size, character);
        return x;
    }

    std::string topHeadingMenu(std::string menuName, char character, int_u size) // internal helping function
    {
        std::stringstream finalData;
        finalData << _menuHeadingChar(size, character) << menuName << _menuHeadingChar(size, character);
        return finalData.str();
    }

    /*
    Clears Console window
    */
    void clear() // // internal helping function (borrowed from the internet (don't remeber from where) (not good at Windows.h headerfile yet)
    {
        COORD topLeft = { 0, 0 };
        HANDLE console = GetStdHandle(STD_OUTPUT_HANDLE);
        CONSOLE_SCREEN_BUFFER_INFO screen;
        DWORD written;
        GetConsoleScreenBufferInfo(console, &screen);
        FillConsoleOutputCharacterA(console, ' ', screen.dwSize.X * screen.dwSize.Y, topLeft, &written);
        FillConsoleOutputAttribute(console, FOREGROUND_GREEN | FOREGROUND_RED | FOREGROUND_BLUE, screen.dwSize.X * screen.dwSize.Y, topLeft, &written);
        SetConsoleCursorPosition(console, topLeft);
    }

    char_u Display::displayMainMenu()
    {
        Sleep(1000); // holds the console for 1 sec
        clear();
        char_u optionArray[8];
        for (int_u i = 0; i < 8; i++) optionArray[i] = 49 + i;
        char_u result;
        bool exists = false;
        string_vec menuItems{ "CREATE AN ACCOUNT", "DIPOSITE AMOUNT", "WITHDRAW AMOUNT", "BALANCE ENQUIRE", "VIEW LIST OF ALL ACCOUNT HOLDERS", "CLOSE AN ACCOUNT", "MODIFY AN ACCOUNT", "EXIT" };
        std::cout << "\r\n\n\t\tMAIN MENU" << std::endl << std::endl;
        for (int_u i = 0; i < menuItems.size(); i++) std::cout << "\t\t0" << i + 1 << "  " << menuItems.at(i) << std::endl << std::endl;
        do
        {
            std::cout << "\n\n\t\tPlease select <1-8> to operate:\t\t";
            result = _getch(); // gets hidden input _getch(), echos input _getche() [from conio.h]
            for (int i = 0; i < 8; i++)
            {
                if (optionArray[i] == result)
                {
                    exists = true;
                    break;
                }
            }
        } while (exists == false);
        return result;
    }

    void Display::coverMenu()
    {
        std::cout << "\n\n\n\n\n\n\n\n\n\n\t\t\tWelcome to ClassicBankCpp: A Banking Management System by Alex\n\n"
            << "\t\t\tPlease Press 'Enter' To Continue...\n\n\n\t\t\t";
        std::cin.get();
        Sleep(3000);
        clear();
    }

    void displayTransectionAction(std::string actionName) // internal helping function
    {
        std::cout << "\n\n\t\tEnter the amount to " << actionName << ":\t";
    }

    void Display::accountStatus(BankInfo* bankInfo)
    {
        std::cout << "\r\n\n\n\n\t\t" << topHeadingMenu("ACCOUNT STATUS", '-', 5)
        << "\n\n\n\t\t" << "Account Number:\t" << bankInfo->acNum << "\n\t\t" << "Account Holder:\t" << bankInfo->acName
        << "\n\t\t" << "Account Type:\t" << bankInfo->acType << "\n\t\t" << "Amount:\t\t" << bankInfo->acBalance << std::endl;
    }

    void Display::displayAccountHolderList(bank_Vec& accountContainer)
    {
        std::string heading = "List of All the Account Holders";
        std::string space(heading.size() * 3, '-');
        std::cout << "\n\n\n" << std::setw(heading.size() * 2) << heading << std::setw(heading.size() * 2) << "\n\n" << space << "\nAc No." << std::setw(20) << "Name"
        << std::setw(30) << "Type" << std::setw(38) << "Amount\n" << space << std::endl;
        for (int_u i = 0; i < accountContainer.size(); i++)
        {
            std::cout << accountContainer[i].acNum << std::setw(28) << accountContainer[i].acName << std::setw(20) << accountContainer[i].acType << std::setw(39) << accountContainer[i].acBalance << "\n";
        }
        std::cout << space << std::endl;
    }


    Functions::Functions() : accountCollection{} {}
    Functions::~Functions() {}

    Functions* f = new Functions(); // creating an intance of Fuctions() class since f->accountCollection is a non-static member of Fuctions() class

    BankInfo Functions::getAccountVec(BankInfo* bankInfo)
    {
        f->accountCollection = UtilityTools::File::readFromDatFile(); // reading data from the stored file
        if (f->accountCollection.size() > 0)
        {
            int pos = search(f->accountCollection, bankInfo);
            if (pos != -1)  return f->accountCollection[pos];
            else return BankInfo();
        }
        else return BankInfo();
    }

    void Functions::createNewAccount()
    {
        Sleep(1000);
        clear();        
        std::string accountInfo[4]; //string array
        f->accountCollection = UtilityTools::File::readFromDatFile();
        std::cout << "\r\n\n\n\n\t\t" << topHeadingMenu("Register New Account", '*', 5);
        do
        {
            std::cout << "\n\n\n\t\tEnter the Account Number:\t\t\t";
            std::getline(std::cin, accountInfo[0]); // account number
        } while (accountInfo[0].length() != 6 || isDigit(accountInfo[0]) == false || accountInfo[0] == "" || exists(f->accountCollection, accountInfo[0]));
        do
        {
            std::cout << "\n\t\tEnter the Name of the Account Holder:\t\t";
            std::getline(std::cin, accountInfo[1]); // account name
        } while (accountInfo[1] == "");
        do
        {
            std::cout << "\n\t\tEnter the Type <S/C>:\t\t\t\t";
            std::getline(std::cin, accountInfo[2]); // account type
            if (accountInfo[2] == "C" || accountInfo[2] == "S") break;
        } while (true);
        do
        {
            std::cout << "\n\t\tEnter the Amount:\t\t\t\t";
            std::getline(std::cin, accountInfo[3]); // amount
        } while (isDigit(accountInfo[3]) == false || accountInfo[3] == "");

        f->accountCollection.push_back(BankInfo(accountInfo[0], accountInfo[1], accountInfo[2], accountInfo[3]));
        UtilityTools::File::writeToDatFile(f->accountCollection); // writing data to a file
        std::cout << "\n\n\t\t\tRecord Updated!!!";
    }


    void Functions::updateAccount(BankInfo* bankInfo, bool withdraw)
    {
        Sleep(1000);
        clear();
        int amountBefore, amountAfter;
        std::string newAmount, message;
        BankInfo acc_data = Functions::getAccountVec(bankInfo);
        message = "Deposite to Account";
        if (withdraw == true) message = "Withdarw from Account";
        std::cout << "\n\n\n\n\t\t" << topHeadingMenu(message, '*', 5);
        Display::accountStatus(&acc_data);
        amountBefore = atoi(acc_data.acBalance.c_str()); //atoi() converting const char* (cString) to int  
        displayTransectionAction(toUpper(UtilityTools::StringManip::split(
            message.c_str(), ' ', UtilityTools::StringManip::splitOption::noEmpty)[0]));
        std::cin >> newAmount;
        std::cin.ignore();
        amountAfter = atoi(newAmount.c_str());
        if (withdraw == true) amountBefore -= amountAfter;
        else amountBefore += amountAfter;
        int pos = search(f->accountCollection, bankInfo);
        f->accountCollection = UtilityTools::File::readFromDatFile();
        f->accountCollection[pos].setBalance(std::to_string(amountBefore));
        UtilityTools::File::writeToDatFile(f->accountCollection);
        std::cout << "\n\n\t\t\tRecord Updated!!!";
    }

    void Functions::displayAccountStatus(BankInfo* bankInfo)
    {
        Sleep(1000);
        clear();
        BankInfo acc_data = Functions::getAccountVec(bankInfo);
        Display::accountStatus(&acc_data);
    }

    void Functions::displayAccountHolders()
    {
        Sleep(1000);
        clear();
        f->accountCollection = UtilityTools::File::readFromDatFile();
        Display::displayAccountHolderList(f->accountCollection);
    }

    void Functions::deleteAccount(BankInfo* bankInfo)
    {
        Sleep(1000);
        clear();
        f->accountCollection = UtilityTools::File::readFromDatFile();
        int pos = search(f->accountCollection, bankInfo);
        f->accountCollection.erase(f->accountCollection.begin() + pos);
        UtilityTools::File::writeToDatFile(f->accountCollection);
        std::cout << "\n\n\t\t\tRecord Updated!!!";
    }

    void modifyAcUpdater(std::string printHead, std::string& cinData, bank_Vec& collections, int_u& vecCollPos, int_u vecPos, BankInfo* bankInfo) // internal helping function
    {
        char_vec data{ 'S', 'M', 'N' };

        if (vecPos == 0)
        {
            do
            {
                std::cout << "\n\n\t\tPlease enter new " << printHead << ":\t";
                std::getline(std::cin, cinData);
            } while (cinData.length() != 6 || isDigit(cinData) == false || cinData == "" || exists(collections, cinData));
        }
        else if (vecPos == 1)
        {
            do
            {
                std::cout << "\n\n\t\tPlease enter new " << printHead << ":\t";
                std::getline(std::cin, cinData);
            } while (cinData == "");
        }
        else if (vecPos == 2)
        {
            while (true)
            {
                std::cout << "\n\n\t\tPlease enter new " << printHead << ":\t";
                std::getline(std::cin, cinData);
                if (cinData == "C" || cinData == "S") break;
            }
        }
        else
        {
            do
            {
                std::cout << "\n\n\t\tPlease enter new " << printHead << ":\t";
                std::getline(std::cin, cinData);
            } while (isDigit(cinData) == false || cinData == "");
        }
        std::cout << "\n\n\t\tPress \"M\" to modify again\n\t\tPress \"S\" save and exit\n\t\tPress \"N\" to exit without saving\n\t\t>>> ";
        char userAns = std::cin.get();
        std::cin.ignore();
        if (exists(data, userAns))
        {
            if (userAns == 'M') Functions::modifyAccount(bankInfo);
            else if (userAns == 'S')
            {
                if (vecPos == 0) collections[vecCollPos].acNum = cinData;
                else if (vecPos == 1) collections[vecCollPos].acName = cinData;
                else if (vecPos == 2) collections[vecCollPos].acType = cinData;
                else collections[vecCollPos].acBalance = cinData;
                std::cout << "\n\n\t\t\tRecord Updated!!!";
            }
            else return;
        }
    }

    void Functions::modifyAccount(BankInfo* bankInfo)
    {
        Sleep(1000);
        clear();
        BankInfo acc_data = Functions::getAccountVec(bankInfo);
        int_u pos = search(f->accountCollection, bankInfo);
        std::string newVar;
        int userChoice;
        std::string data[]{ "Account Number", "Holder's Name", "Account Type", "Account Balance" };
        std::cout << "\n\n\n\n\t\t" << topHeadingMenu("Modify An Account Information", '*', 5);
        for (int i = 0; i < 4; i++) std::cout << "\n\n\t\tPlease press " << i + 1 << " to edit " << data[i] << std::endl;
        std::cout << "\n\n\t\t>>> ";
        std::cin >> userChoice;
        std::cin.ignore();
        if (exists(range(1, 4), userChoice))
        {
            if (userChoice == 1) modifyAcUpdater(data[0], newVar, f->accountCollection, pos, 0, bankInfo);
            else if (userChoice == 2) modifyAcUpdater(data[1], newVar, f->accountCollection, pos, 1, bankInfo);
            else if (userChoice == 3) modifyAcUpdater(data[2], newVar, f->accountCollection, pos, 2, bankInfo);
            else modifyAcUpdater(data[3], newVar, f->accountCollection, pos, 3, bankInfo);
        }
        UtilityTools::File::writeToDatFile(f->accountCollection);
    }

    BankInfo Functions::getAccountNumber()
    {
        Sleep(1000);
        clear();
        std::string acNum;
        BankInfo bankInfo;
        do
        {
            std::cout << "\n\n\t\tPlease enter the account number:\t\t";
            std::getline(std::cin, acNum);
        } while (acNum == "");
        f->accountCollection = UtilityTools::File::readFromDatFile();
        if (f->accountCollection.size() > 0)
        {
            if (exists(f->accountCollection, acNum))
            {
                bankInfo.setAcNum(acNum);
                return bankInfo;
            }
            else
            {
                std::cout << "\n\t\tNot found!\n";
                return bankInfo;
            }
        }
        else
        {
            std::cout << "\n\t\tNo Account has been created!!!\n";
            return bankInfo;
        }
    }

    void Functions::appExit()
    {
        exit(EXIT_SUCCESS);
    }

    void Functions::run()
    {
        Display::coverMenu();
        bool continueProgram = true;

        while (continueProgram)
        {
            char_u userChoice = Display::displayMainMenu();
            BankInfo acNum;
            switch (userChoice)
            {
            case '1':
                Functions::createNewAccount();
                break;
            case '2':
                acNum = Functions::getAccountNumber();
                if (acNum.acNum != "0")
                {
                    Functions::updateAccount(&acNum);
                    break;
                }
                else break;
            case '3':
                acNum = Functions::getAccountNumber();
                if (acNum.acNum != "0")
                {
                    Functions::updateAccount(&acNum, true);
                    break;
                }
                else break;
            case '4':
                acNum = Functions::getAccountNumber();
                if (acNum.acNum != "0")
                {
                    Functions::displayAccountStatus(&acNum);
                    std::cin.ignore();
                    break;
                }
                else break;
            case '5':
                Functions::displayAccountHolders();
                std::cin.ignore();
                break;
            case '6':
                acNum = Functions::getAccountNumber();
                if (acNum.acNum != "0")
                {
                    Functions::deleteAccount(&acNum);
                    break;
                }
                else break;
            case '7':
                acNum = Functions::getAccountNumber();
                if (acNum.acNum != "0")
                {
                    Functions::modifyAccount(&acNum);
                    break;
                }
                else break;
            case '8':
                Functions::appExit();
                break;
            }
        }
    }
}

main.cpp

#include "bankclassic.h"

int main()
{
    BankClassic::Functions::run();
    std::cin.get();
    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ I edited my answer if you are interested. Also you should accept Edward's answer \$\endgroup\$ – brug May 22 '18 at 0:54
8
\$\begingroup\$

I see a number of things that may help you improve your program.

Use the required #includes

The code uses std::find which means that it should #include <algorithm>. It might compile without it, but if it does, it will only be because some other header happens to include it, and that could change with the next version of your compiler and is certainly going to vary by compiler.

Use appropriate types

It seems odd that the account balance is stored as a string. I'd have expected a numeric value for that. One common idiom is to use an int and have it store the number of cents, so if the account has $123.45 the value would simply be 12345. This has the advantage of actually being a numeric type (that could be zero or negative) and avoids rounding problems associated with using a float or double type.

Avoid passing pointers

In modern C++, passing around raw pointers is usually not the best solution. For example, the Display class contains this function:

static void accountStatus(BankInfo* bankInfo);

This is odd in a number of ways. First, I'd expect that this would be a function of the BankInfo class and not the Display class. Second, I'd expect that this function would not alter the passed BankInfo instance, so that should be declared const.

Use standard functions

The code currently contans a number of calls to Sleep which is a Windows-only function. I'd suggest that using standard C++ functions where practical would make the program more portable, so I'd suggest replacing lines like this:

Sleep(1000);

with the equivalent standard function:

std::this_thread::sleep_for(std::chrono::seconds(1));

This will require a C++ compliant compiler and <thread> and <chrono>. Alternatively, isolate the platform-specific code in a function and then call that function. Better yet, see the next suggestion.

Think of the user

There are a number of things here that are not very user friendly. All of the delays, for example, force the user to wait. Also, apparently the user account number needs to be exactly six digits long, but this is not mentioned in the prompt.

Prefer modern initializers for constructors

The constructors for BankInfo can be converted to use the C++11 parameter intialization style.

BankInfo::BankInfo(std::string accountNumber, std::string accountName, std::string accountType, std::string accountBalance) :
    acNum{accountNumber},
    acName{accountName},
    acType{accountType},
    acBalance{accountBalance}
{}

Also, I'd suggest that making all of those const std::string & would allow a more flexible usage.

Rethink your classes

A class should have a clear, well defined purpose. The BankInfo class almost meets that definition, but is a struct instead of a class and is not well named. Rather than BankInfo, I'd probably call it Account which seems a more accurate description. Also, because it's a struct, it doesn't make much sense to have setter functions -- any other class can simply write directly to the member data. I'd also say that rather than having a Functions class and a File class (both poor names), I'd suggest instead an AccountCollecton class.

Don't abuse typedefs

There might be a reason to declare this:

typedef std::vector<BankInfo> bank_Vec;

But these are just obfuscation and impede understanding of the program by other programmers reading your code:

typedef std::vector<char> char_vec;
typedef unsigned char char_u;
typedef unsigned int int_u;

Separate user I/O from logic

There probably needs to be something like a Menu object which presents choices and gets input from the user. It would also be useful to have per-field validation for each kind of data item. All of this would greatly streamline and declutter the main program flow.

Fix spelling errors

The code has Deposite instead of deposit and Withdarw instead of withdraw. These kinds of typos don't bother the compiler at all, but they will bother human readers of the code and make it a little more difficult to understand and maintain.

Avoid one-line if statements

It's generally better, and especially for beginners, not to crowd things onto a single line. So for example, instead of this:

if (split_option == splitOption::all) splited_data.push_back(token);

I'd recommend writing this:

if (split_option == splitOption::all) {
    splited_data.push_back(token);
}

If you get into the habit of always explicitly writing the braces, you'll be much less likely to encounter a bug due to missing braces.

Use a portable clear()

Instead of restricting things to Windows only, one can often achieve a much more portable result using ANSI Escape sequences, which are widely supported (Linux, Windows, Mac).

void cls()
{
    std::cout << "\x1b[2J";
}

It's shorter, simpler, and more portable. What more could you want?

Let the compiler generate defaults

Instead of writing empty constructors and destructors like these:

Display::Display() {}
Display::~Display() {}

let the compiler generate them by simply deleting those lines. Or if you want to make sure that they're generated as the defaults (and more importantly, signalling to the reader what is happening), you could do this in the declaration:

Display() = default;
~Display() = default;

Use const references where practical

The code currently declares a display function like this:

static void displayAccountHolderList(bank_Vec& accountContainer);

Better, since the underlying accountContainer is not altered, would be to declare the parameter const:

static void displayAccountHolderList(const bank_Vec& accountContainer);
\$\endgroup\$
  • \$\begingroup\$ very useful suggestions, thanks for taking time to review my code. \$\endgroup\$ – A J May 7 '18 at 21:38
  • \$\begingroup\$ One the one hand you expect that accountInfo is part of the BankInfo class, on the other hand you suggest to separate IO from the logic. That's a bit confusing. \$\endgroup\$ – Roland Illig May 8 '18 at 5:52
  • \$\begingroup\$ "One common idiom is to use an int and have it store the number of cents." Can you elaborate on this a bit? Does it use two values, one for the dollar amount and one for the cent amount? \$\endgroup\$ – yuri May 8 '18 at 8:17
  • \$\begingroup\$ @RolandIllig: I'm not understanding what part of that is confusing. If you can clarify, I may be able to improve my answer. \$\endgroup\$ – Edward May 13 '18 at 18:45
  • \$\begingroup\$ The accountInfo handles IO. Therefore it should not be part of the BankInfo class, to avoid coupling the class to this specific output format. Yet you suggest to do exactly that, if I understood you correctly. \$\endgroup\$ – Roland Illig May 14 '18 at 17:28
2
\$\begingroup\$

Edward did an excellent job reviewing your code and I would just like to add one thing.

Do away with useless comments

You have fairly readable code. Very much so for a beginner and your comments take away from that. Comments are supposed to explain why you chose to do something a certain way. And even then only if it isn't reasonably obvious. You aren't writing a tutorial on how the language works and its safe to assume that anyone reading your C++ source code probably knows how to read C++.

EDIT:

Your defense of your documentation led to me looking deeper into the nature of software documentation. At the end of the day I discovered a.) You're not wrong and b.) neither am I.

That probably sounds stupid so let me expand. Documentation is good. It is important and useful. And it can be redundant.

/*
Runs the application
*/
static void run();

This is definitely over-explanation. It is okay to know your audience. If your documentation is meant to be read by future program maintainers then there is a degree of being able to read code that you can expect from them. Even if your documentation is for a lay-person the run() function should still speak for itself. Also thank you for leading me down this path of discovery.

See this post for some important information on the subject.

\$\endgroup\$
  • \$\begingroup\$ It was not a comment, in python it’s called docstring, which describes what a function does(doesn’t matter if it has a comprehensive name). And if I’m not wrong in c++ docstring is done the way I did (like using <summary> in c#. But thanks for ur feedback. Btw, i did use the function (isDigit()). \$\endgroup\$ – A J May 8 '18 at 2:37
  • \$\begingroup\$ If the function were called isAllDigits, I would agree that the comment is redundant. With the current name the comment is helpful. It should include another example though: what happens for the empty string? \$\endgroup\$ – Roland Illig May 8 '18 at 5:56
  • \$\begingroup\$ @RolandIllig well, as I mentioned earlier they are docstring not comments. IsDigit(“”) will return false. \$\endgroup\$ – A J May 8 '18 at 8:58

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.