School Banking Project

How can I optimize this code

Header File:

#pragma once
#ifndef Account_h
#define Account_h

#include "stdafx.h"
#include 
#include 
#include 
#include 
#include 
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/20

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  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 > Nf;
            cout > Nm;
            cout > Nl;
            cout << Nl << endl;
            Bank[i].setName(Nf, Nm, Nl);

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

StackExchange Code Review Q#130066, answer score: 6