3
\$\begingroup\$

How can I optimize this code

Header File:

#pragma once
#ifndef Account_h
#define Account_h

#include "stdafx.h"
#include <iostream>
#include <string>
#include <fstream>
#include <iomanip>
#include <assert.h>
using namespace std;

struct Birth_Date
{
    int Day;
    int Month;
    int Year;

};
struct Name
{
    // Name ADT
    string First_name;   // Person First Name
    string Middle_name; // Person Middle Name
    string Last_name; // Person Last Name
};
struct Balance
{
    float initial_saving;
    float initial_checking;
};
// Class
class CUSTOMER
{
private:
    // struct variables
    Birth_Date birth_date;
    Name name;
    Balance balance;


public:
    //  void withdraw(float amount, int  account_type);
    //  void deposit(float amount, int  account_type);
    //void check_balance() {} // print out the balance on screen
    CUSTOMER();   // default constructor
    // used to create instance varible, initialize for usage.
                  // CUSTOMER(Birth_Date birthDate, Balance bal, Name  NAME); // parameterized constructor
                  // Get Functions
    Name getName() { return name; }
    Balance getBalance() { return balance; }
    Birth_Date getBirthDate() { return birth_date; }

    // Set Functions
    // Name Function
    void setName(string a, string b, string c)
    {
        name = { a, b, c };        // Set value of a to first name

    }
    // Date Function
    void setDate(int a, int b, int c)
    {
        assert(a >= 0 && b >= 0 && c >= 0);

        birth_date = { a, b, c };
    }
    // Balance set Function
    void setBalance(float a, float b)
    {
        assert(a >= 0 && b >= 0);
        balance = { a, b };
    }
};

#endif

Source code:

/ Project 3 Classic.cpp : Defines the entry point for the console application.
//   Purpose:  This program enables Banks and their workers to help a client with transactions like: Opening new account, Deposit, withdrawal, check balance.
//   Author: Emmanuel Obi
//   Creation Date: 4/1/2016
//   Modification Date: 4/18/2016
//   Input: Data from input file (account.dat)
//   Output: Updated or new information to output file(updated_account.dat)
//   Precondition:   Parameters of class member functions must be true.
//   Postcondition:   Functions must be true after running code.
//   Keyword: Class, struct, fstream, string.
//

#include "stdafx.h"
#include <iostream>
#include <string>
#include <fstream>
#include <iomanip>
#include <assert.h>
#include "Account.h"
using namespace std;


CUSTOMER::CUSTOMER()
{
    name = { " ", " " , " " };
    birth_date = { 0, 0, 0 };
    balance = { 0.0, 0.0 };
}
/* Temp
CUSTOMER::CUSTOMER(Birth_Date birthDate, Balance bal, Name  NAME)
{
name = NAME;
birth_date = birthDate;
balance = bal;

}
*/
//int search(string a, string b, int len);       // Prototype
// Customer is a variable of class CUSTOMER
CUSTOMER Bank[1000000];
ifstream myInfile;
ofstream myOutfile;

// Cutomer Search

int main()
{
    cout << setw(70);   // Text Alignment
    cout << "Welcome to Emmanuel's Bank" << endl;     // This can be edited for company's use
    int count = 0;  //Index Variable
    myInfile.open("account.dat");
    cout << "Please wait while your file is being read. " << endl;
    // Variables for struct Name
    string Nf = " ";
    string Nm = " ";
    string Nl = " ";
    // Variables for struct Date
    int Dm = 0;
    int Dy = 0;
    int Dd = 0;
    // Variables for struct Balance
    float B_check;
    float B_savings;
    // Read user input file
    while (!myInfile.eof())    // If file is open
    {

        myInfile >> Dm >> Dd >> Dy;
        Bank[count].setDate(Dd, Dm, Dy);

        myInfile >> Nf >> Nm >> Nl;             // Read  date from input file

        Bank[count].setName(Nf, Nm, Nl);    // Function for Name

        myInfile >> B_savings;
        myInfile >> B_check;

        Bank[count].setBalance(B_savings, B_check);
        cout << Bank[count].getName().First_name << "       " << Bank[count].getName().Middle_name << "       " << Bank[count].getName().Last_name << endl;  // Debugging Purpose
        count++; //Increment index 
    }
    cout << " Your files were sucessfully read. " << endl;
    myInfile.close();
    myOutfile.open("updated_account.dat");   // Open output file
    char select = ' ';    // User selection 
                       // Requesting user selection
    cout << "What do you want to do today ? " << endl;
    cout << " Type 1 for customer new account ? " << endl;
    cout << " Type 2 for customer check balance ? " << endl;
    cout << " Type 3 for customer withdrawal ? " << endl;
    cout << " Type 4 for customer deposit ? " << endl;

    cin >> select;
    string Fn, Mn, Ln; // Variable s for first name and last name
    int account; // Account type
    int number = 0; // Number of customers
    float AMOUNT = 0.0;

    // New user creation
    bool flag = true;
    while (flag==true)
    {
        cout << " Do you want to create a new customer account ? (y or n) " << endl;
        cin >> select;
        if (select == 'y' || select == 'Y')
        {

            cout << "How many customer's account will you like to create ? " << endl;
            cin >> number;

            for (int i = 0; i < number; i++)
            {
                cout << "Type in customer's name: " << endl;
                cout << " Type in first name: " << endl;
                cin >> Nf;
                cout << Nf << endl;
                cout << " Type in middle name: ";
                cin >> Nm;
                cout << Nm << endl;
                cout << " Type in last name: " << endl;
                cin >> Nl;
                cout << Nl << endl;
                Bank[i].setName(Nf, Nm, Nl);

                cout << "Type in customer's date of birth: " << endl;
                cin >> Dm >> Dd >> Dy;
                cout << Dm << Dd << Dy << endl;
                Bank[i].setDate(Dm, Dd, Dy);

                cout << " Type in customer's entry savings:  ";
                cin >> B_savings;
                cout << " Type in customer's entry deposit, if any: ";
                cin >> B_check;
                Bank[i].setBalance(B_savings, B_check);
                cout << "What's customer account type  (type 2 for savings or 3 for checking) : " << endl;
                cin >> account;

                //Output to ouput file
                myOutfile << "New Customer" << endl;
                myOutfile << "Birth Date: " << Bank[i].getBirthDate().Month << Bank[i].getBirthDate().Day << Bank[i].getBirthDate().Year << endl;
                myOutfile << "Name: " << Bank[i].getName().First_name << " " << Bank[i].getName().Middle_name << " " << Bank[i].getName().Last_name << endl;
                myOutfile << "Savings: $" << Bank[i].getBalance().initial_saving << endl;
                myOutfile << "Checking: $" << Bank[i].getBalance().initial_checking << endl;
                cout << "Customer account successfully created. " << endl;
            }
        }
        else if (select == 'n' || select == 'N')
        {
            flag = false;
            break;
        }
        else
            cout << " Invalid selection" << endl;
    }

    // Checking Balance
    bool flag2 = true;
    while (flag2 == true)
    {
        cout << "How many customers needs to check balance: " << endl;
        cin >> number;
        for (int i = 0; i < number; i++)
        {
            cout << "Type in customer's name: " << endl;
            cout << " Type in first name: ";
            cin >> Nf;
            cout << Nf << endl;
            cout << " Type in last name: ";
            cin >> Nl;
            cout << Nl << endl;
            cout << "What's customer account type  (type 2 for savings or 3 for checking) : " << endl;
            cin >> account;
            if (account == 2)
            {
                int a = 0.0;
                for (int i = 0; i < count; i++)
                {
                    if ((Bank[i].getName().First_name == Nf) && (Bank[i].getName().Last_name == Nl))
                    {
                        a = Bank[i].getBalance().initial_saving;
                        cout << "$" << a << endl;
                    }

                }
            }
            else if (account == 3)
            {
                int a = 0.0;
                for (int i = 0; i < count; i++)
                {
                    if ((Bank[i].getName().First_name == Nf) && (Bank[i].getName().Last_name == Nl))
                    {
                        a = Bank[i].getBalance().initial_checking;
                        cout << "$" << a << endl;
                    }

                }
            }
        }
    }
    bool flag3 = true;
    // Handling Withdrawal
    while  (flag3 == true)
    {
        cout << "How many customers needs a withdrawal: " << endl;
        cin >> number;

        for (int i = 0; i < number; i++)
        {
            cout << "Type in customer's name: " << endl;
            cout << " Type in first name: ";
            cin >> Nf;
            cout << Nf << endl;
            cout << " Type in last name: ";
            cin >> Nl;
            cout << Nl << endl;
            // cout << search(Nf, Nl, number);
            cout << "How much will you like to withdraw ? " << endl;
            cin >> AMOUNT;
            cout << "What's customer account type  (type 2 for savings or 3 for checking) : " << endl;
            cin >> account;

            for (int i = 0; i < count; i++)
            {
                if ((Bank[i].getName().First_name == Nf) && (Bank[i].getName().Last_name == Nl))
                {
                    float x = 0.0;
                    x = Bank[i].getBalance().initial_saving;
                    x = x - AMOUNT;
                    cout << "$" << x << endl;
                    //
                    myOutfile << "Updated Customer" << endl;
                    myOutfile << "Birth Date: " << Bank[i].getBirthDate().Month << " " << Bank[i].getBirthDate().Day << " " << Bank[i].getBirthDate().Year << endl;
                    myOutfile << "Name: " << Bank[i].getName().First_name << " " << Bank[i].getName().Middle_name << " " << Bank[i].getName().Last_name << endl;;
                    myOutfile << "Savings: $" << x << endl;
                    myOutfile << "Checking: $" << Bank[i].getBalance().initial_checking << endl;
                }
                else {}
            }


        }
    }
    // Deposit
    bool flag4 = true;
    while (flag4 == true)
    {
        cout << "How many customers needs a deposit: " << endl;
        cin >> number;
        for (int i = 0; i < number; i++)
        {
            cout << "Type in customer's name: " << endl;
            cout << " Type in first name: ";
            cin >> Nf;
            cout << Nf << endl;
            cout << " Type in last name: ";
            cin >> Nl;
            cout << Nl << endl;
            // cout << search(Nf, Nl, number);
            cout << "How much will you like to deposit ? " << endl;
            cin >> AMOUNT;
            cout << "What's customer account type  (type 2 for savings or 3 for checking) : " << endl;
            cin >> account;
            for (int i = 0; i < count; i++)
            {
                if ((Bank[i].getName().First_name == Nf) && (Bank[i].getName().Last_name == Nl))
                {
                    float x = 0.0;
                    x = Bank[i].getBalance().initial_saving;
                    x = x + AMOUNT;
                    cout << "$" << x << endl;

                    // 
                    myOutfile << "Updated Customer" << endl;
                    myOutfile << "Birth Date: " << Bank[i].getBirthDate().Month << " " << Bank[i].getBirthDate().Day << " " << Bank[i].getBirthDate().Year << endl;
                    myOutfile << "Name: " << Bank[i].getName().First_name << " " << Bank[i].getName().Middle_name << " " << Bank[i].getName().Last_name << endl;;
                    myOutfile << "Savings: $" << x << endl;
                    myOutfile << "Checking: $" << Bank[i].getBalance().initial_checking << endl;
                }
            }
        }
    }
    myOutfile.close();
    cout << "Thanks for using this application, have a nice day. " << endl;
    system("pause");
    return 0;
}
\$\endgroup\$
6
\$\begingroup\$

Floating Point Not exact

struct Balance
{
    float initial_saving;
    float initial_checking;
};

Floating numbers can not represent all numbers exactly. So best not to use them for things that involve money (people get upset if you loose money, banks get upset if you give it away). Use some form of integer just store the number of cents rather than dollars.

Geter and Setters are a blight

Get/Set expose implementation details and break encapsulation avoid them at all costs. Sometimes they are necessary but usually not. For example I bet the only time you use the getters is print out the object. Its better to write a print function rather than the getters.

Your methods should be actions that are applied to your object. Thus method names tend to be verbs (actions).

If you must have getters then return by reference (so you don't waste time copying the vaules if you don't need to.

Name const&       getName()      const { return name; }
Balance const&    getBalance()   const { return balance; }
Birth_Date const& getBirthDate() const { return birth_date; }

Also how often do you change the date of birth (that should never happen so there should be no method for it), changing name can happen but it should be rare as a result you would want to make it harder to call accidentally.

Do you really want a method that allow you to change the balance? Normally I would see a method that spends money or saves money. These would update the balance appropriately but they would not directly set the balance. Currently If I spend money I have to call the get method to get the balance then update the balance then call set to update the object. This just screams of encapsulation issues and something that should be done inside the object as its own method.

Bad Constructor

The reason you need a default constructor (and thus setters) is because you are using an Array.

 CUSTOMER Bank[1000000];

None of this would be needed if you used a std::vector

 std::vector<CUSTOMER>  bank;

Naming Conventions

Identifiers in "All Caps" are usually reserved for macros. Using them for other purposes is dangerous. Don't do it. Your need to use macros has mostly been replaced by C++ features so it is unlikely you will need to use macros (apart from include guards) so you should not have identifiers that are all caps.

Identifiers with an initial upper case letter are usually used to indicate a user defined type.

Identifiers with an initial lower case letter are usually used to indicate an object (variable or function).

#ifndef Account_h     // Should be all caps
class CUSTOMER        // Should not be all caps Customer is better.
string First_name;    // first letter better lowercase so we can see its an object.

Iterating over a file

This is nearly always wrong.

while (!myInfile.eof()) 

And in your case it is also wrong. You can read all about it here: Why is iostream::eof inside a loop condition considered wrong?

Namespace using

This should absolutely never be in a header file.

using namespace std;

If I include your header file it can break my code. So doing this will get your code banned from opens source projects.

You should also avoid it in source files. Using it is just asking for your code to break. A good read is here Why is “using namespace std” in C++ considered bad practice?

Self Documenting Code

Your main function is one log gruesome read. You should divide this into multiple functions. Doing this documents what each section of code does with the name of the function. Thus I don't need to keep the whole program in my head. I just need to validate a function in isolation then when I see it called I know it does one thing.

This will also help you DRY up your code by removing repeated code into its own function.

This bit of code is repeated several times:

            cout << "Type in customer's name: " << endl;
            cout << " Type in first name: " << endl;
            cin >> Nf;
            cout << Nf << endl;
            cout << " Type in middle name: ";
            cin >> Nm;
            cout << Nm << endl;
            cout << " Type in last name: " << endl;
            cin >> Nl;
            cout << Nl << endl;
            Bank[i].setName(Nf, Nm, Nl);

Put it in a function so it is only done once.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Good review. The only things I'd add is that rather than Customer, the name of the object should probably be Account and that the "magic numbers" should really be named constants or enums and that system("pause") is evil and should never be used. \$\endgroup\$ – Edward Jun 3 '16 at 23:59

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.