5
\$\begingroup\$

I'm looking for feedback on this Personal Finance tool. I want to know what I've done well and what I haven't done so well. Please don't criticise the fact I've used Hungarian notation, it's required in my university assignment and I know I probably should be using classes but at the moment I'm just sticking to the procedural approach.

Scenario

A reputable bank has asked you to create a personal finance management program. The program will need to be able to take the user's monthly wage, their monthly bills, and any weekly bills that they may occur. This will then be broken down to find out how much of their wages they will then have left to save.

The user should be able to enter any amount of monthly bills and weekly bills. The user should also have the option to add other users from their household to be handled within the calculations.

Once all of the relevant information has been included, an overview of the bills against a weekly, monthly and yearly cost should be output.

Inputs

  • User's Name (Must be more than 1)
  • Monthly Wage (Must be more than 1)
  • The different bills the user has to pay (Per Person)

Outputs

  • User's Name
  • Weekly, Monthly, Yearly Wage Total
  • Weekly, Monthly, Yearly Bills Total
  • Total Spent on Bills
  • Total left to Save
  • 10% over and under the total that can be saved
  • How much can be saved per month
  • 10% over and under the total that can be saved per month

The code

Personal Finance Management.cpp

This file contains the main function. Program execution begins and ends there.

#include <iostream>
#include <string>
#include <vector>
#include <cctype>
using namespace std;

void Addusers(vector<string>&sUserName)
{
    string sUserNameInput = "";
    cout << "\nEnter the name of household member: ";
    cin >> sUserNameInput;
    sUserName.push_back(sUserNameInput);
}
void AddMonthlyWage(vector<string>&sUserName,vector<double> &dMonthlyWage)
{
    double dInputMonthlyWage = 0;
    cout << "\n" << *sUserName.rbegin() << "'s monthly income: ";
    cin >> dInputMonthlyWage;
    dMonthlyWage.push_back(dInputMonthlyWage);
}

void YearlyAmount(vector<double>&dYearlySalary,vector<double>&dMonthlyWage, vector<string>&sUserName)
{
    double dYearlySalaryProcess = 0;
    dYearlySalaryProcess = *dMonthlyWage.rbegin() * 12;
    dYearlySalary.push_back(dYearlySalaryProcess);
}

void WeeklyAmount(vector<double>&dYearlySalary, vector<double>&dWeeklySalary, vector<string>&sUserName)
{
    double dWeeklySalaryProcess = 0;
    for (int iCount = 0; iCount != sUserName.size(); iCount++)
    {
    dWeeklySalaryProcess = (*dYearlySalary.rbegin() / 12) / 4;
    dWeeklySalary.push_back(dWeeklySalaryProcess);
    }
}

double dCalculateBillAmounts(double dYearlySalary,const double dIncomeTax, double dWaterBill, double dElectricityBill)
{
    double dTotalBillProcess = 0;
    dTotalBillProcess = (dYearlySalary * dIncomeTax) + dWaterBill + dElectricityBill;
    return dTotalBillProcess;
}

void BillAmounts(vector<double> &dYearlySalary, vector<string>&sUserName, vector<double>&dTotalBill)
{
    const double dIncomeTax = 0.1;
    vector<double> dWaterBill;
    vector<double> dElectricityBill;
    double dTotalBillProcess = 0;
    double dInputBill = 0;

    cout << "\nPlease enter " << *sUserName.rbegin() << "'s water bill: ";
    cin >> dInputBill;
    dWaterBill.push_back(dInputBill);


    cout << "\nPlease enter " << *sUserName.rbegin() << "'s electricity bill: ";
    cin >> dInputBill;
    dElectricityBill.push_back(dInputBill);

    dTotalBillProcess = dCalculateBillAmounts(*dYearlySalary.rbegin(), dIncomeTax, *dWaterBill.rbegin(), *dElectricityBill.rbegin());
    dTotalBill.push_back(dTotalBillProcess);

}
double CalculateMonthly(double dAmount)
{
    double dCalculateMonthlyProcess = dAmount / 12;
    return dCalculateMonthlyProcess;
}
double CalculateWeekly(double dAmount)
{
    double dCalculateWeeklyProcess = dAmount / 54;
    return dCalculateWeeklyProcess;
}
double TotalOver(double dAmount, double dPercentage)
{
    double dPercentageUnder = dAmount + (dAmount * dPercentage);
    return dPercentageUnder;
}
double TotalUnder(double dAmount, double dPercentage)
{
    double dPercentageOver = dAmount - (dAmount * dPercentage);
    return dPercentageOver;
}

void DisplayResults(vector<string> &sUserName, vector<double> &dMonthlyWage, vector<double> &dTotalBill, vector<double> &dYearlySalary,vector<double> &dWeeklySalary)
{
    vector<double> dTotalSavings;
    double dTotalSavingsProcess = 0;
    double dSavingsOverProcess = 0;
    double dSavingsUnderProcess = 0;
    vector<double> dSavingsOverPercentage;
    vector<double> dSavingsUnderPercentage;
    const double dPercentage = 0.1;

    vector<double> dBillsOverPercentage;
    vector<double> dBillsUnderPercentage;
    double dBillsOverProcess = 0;
    double dBillsUnderProcess = 0;

    for (int iCount = 0; iCount != sUserName.size(); iCount++)
    {
        cout << "\nResults for - " << sUserName.at(iCount) << ".";
        cout << "\n--Salary--";
        cout << "\nYearly salary is: £" << dYearlySalary.at(iCount) << ".";
        cout << "\nMonthly salary: £" << dMonthlyWage.at(iCount) << ".";
        cout << "\nWeekly salary is: £" << dWeeklySalary.at(iCount) << ".";
        cout << "\n--Bills--";
        cout << "\nTotal spent on bills: £" << dTotalBill.at(iCount) << ".";
        cout << "\nMonthly bill cost: £" << CalculateMonthly(dTotalBill.at(iCount)) << ".";
        cout << "\nWeekly bill cost: £" << CalculateWeekly(dTotalBill.at(iCount)) << ".";

        cout << "\n--Savings over and under 10%--";
        dTotalSavingsProcess = dYearlySalary.at(iCount) - dTotalBill.at(iCount);
        dTotalSavings.push_back(dTotalSavingsProcess);
        cout << "\nTotal savings: £" << dTotalSavings.at(iCount);

        dSavingsOverProcess = TotalOver(dTotalSavings.at(iCount), dPercentage);
        dSavingsOverPercentage.push_back(dSavingsOverProcess);
        cout << "\n" << dPercentage * 100 << "% over their total savings: £" << dSavingsOverPercentage.at(iCount);

        dSavingsUnderProcess = TotalUnder(dTotalSavings.at(iCount),dPercentage);
        dSavingsUnderPercentage.push_back(dSavingsUnderProcess);
        cout << "\n" << dPercentage * 100 << "% under their total savings: £" << dSavingsUnderPercentage.at(iCount);
        cout << "\n--Bills over and under 10%--";
        dBillsOverProcess = TotalOver(dTotalBill.at(iCount), dPercentage);
        dBillsOverPercentage.push_back(dBillsOverProcess);
        cout << "\n" << dPercentage * 100 << "% over their total bills: £" << dBillsOverPercentage.at(iCount);

        dBillsUnderProcess = TotalUnder(dTotalBill.at(iCount),dPercentage);
        dBillsUnderPercentage.push_back(dBillsUnderProcess);
        cout << "\n" << dPercentage * 100 << "% under their total bills: £" << dBillsUnderPercentage.at(iCount);
        cout << "\n";
    }
}

void ContinueOptions(bool &bExit)
{
    char cSelection = 0;
    cout << "Do you wish to add another family member? (Y/N) ";
    cin >> cSelection;
    cSelection = toupper(cSelection);
    if(cSelection == 'Y')
    {
        cout << "\nAdd a new family member";
    }
    else
    {
        cout << "\nGoodbye!";
        bExit = true;
    }
}
int main()
{
    vector<string>sUserName;
    vector<double>dMonthlyWage;
    vector<double>dYearlySalary;
    vector<double>dWeeklySalary;
    vector<double>dTotalBills;
    bool bExit = false;
    do
    {
        //Procedure 1
        Addusers(sUserName);
        //Procedure 2
        AddMonthlyWage(sUserName,dMonthlyWage);
        //Procedure 3
        YearlyAmount(dYearlySalary,dMonthlyWage, sUserName);
        //Procedure 4
        WeeklyAmount(dYearlySalary,dWeeklySalary, sUserName);
        //Procedure 5
        BillAmounts(dYearlySalary,sUserName,dTotalBills);
        //Procedure 6
        DisplayResults(sUserName, dMonthlyWage,dTotalBills,dYearlySalary,dWeeklySalary);
        //Procedure 7
        ContinueOptions(bExit);
    }while(bExit == false);
    cout << "\n";
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Is it explicitly required to not to use classes? \$\endgroup\$ – Moia Dec 5 '19 at 17:29
5
\$\begingroup\$

Avoid using namespace std

That's not one of the namespaces designed for wholesale import into the global namespace. Such a using directive denies you the advantages that namespaces bring.

Always check your inputs

See the problem here?

cin >> sUserNameInput;
sUserName.push_back(sUserNameInput);

If the >> operator fails, sUserNameInput will still be empty. But the requirement says it must be greater than 1 (presumably meaning the name length), so we fail.

Always check that >> was successful, either by testing the stream (it has a conversion to bool) or by setting it to throw exceptions (and then handle the exceptions appropriately).

Use const on reference parameters that we're not changing.

For example, YearlyAmount() only needs to read dMonthlyWage, not modify it, and doesn't use sUserName at all:

void YearlyAmount(std::vector<double>& dYearlySalary,
                  std::vector<double> const& dMonthlyWage,
                  std::vector<string>&)

We should probably just remove the unused paramater.

Accessing last element

std::vector() provides back() as a more convenient way to write *v.rbegin().

Lines end in \n

We need to print \n at the end of each line, not at its beginning.

std::toupper() takes an int value

Arguments to character conversion functions take the unsigned value of a character, represented as int. We need to cast to unsigned char before the promotion to int happens, to avoid passing negative values.

Prefer returning a value than writing to a reference.

ContinueOptions has a single return value, so return it. Look how much clearer that is:

bool bExit = false;
do
{
    ...
    ContinueOptions(bExit);
}while(bExit == false);

That becomes:

do
{
    ...
} while(ContinueOptions());

That's with a function like this:

bool ContinueOptions()
{
    std::cout << "Do you wish to add another family member? (y/N) ";
    char cSelection;
    std::cin >> cSelection;
    if (!std::cin) {
        // input failed - assume "no"
        return false;
    }
    bool go_again = std::toupper(static_cast<unsigned char>(cSelection)) == 'Y';
    std::cout << (go_again ? "Add a new family member\n" : "Goodbye!\n");
    return go_again;
}
\$\endgroup\$
  • \$\begingroup\$ Hi there, thank you for reviewing my program. You’ve mentioned a lot of interesting points which I have taken on board. I’m a little lost on your second point about inputs. I can’t see anything wrong? I’d appreciate if you elaborate on that. I’m very glad you told me about the last point because I was thinking to myself there must be a better way. Thanks for that! \$\endgroup\$ – George Austin Bradley Dec 5 '19 at 21:24
  • \$\begingroup\$ Input can go wrong for all sorts of reasons - end of file (or broken connection), input that fails to convert (e.g. entering letters when a number was required), etc. If we always assume input is successful, and never check for errors, we'll get strange results. Look at my version of ContinueOptions for a very simple error check - if (!std::cin). \$\endgroup\$ – Toby Speight Dec 5 '19 at 22:10
  • \$\begingroup\$ Oh I see what you mean! Thanks very much! I greatly appreciate your help! \$\endgroup\$ – George Austin Bradley Dec 5 '19 at 22:59
3
\$\begingroup\$

Complexity

The code does a pretty good job of following the Single Responsibility Principle states:

that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

The exception to this is the function DisplayResults() which is too complex (does too much in one function). Based on the name of the function it should do exactly one thing, which is display the results.

The Addusers() function function should be responsible for calling all the functions to add user information, there should be a new function call AddUserName().

Many of the functions would require less parameters if user was a class and that would greatly simplify the implementation.

In the calculate functions the code can simply return the calculation rather than assigning it to a variable and then returning the variable. It might be possible to write these functions as lambda expressions rather than functions.

Avoid using namespace std

If you are coding professionally you probably should get out of the habit of using the using namespace std; statement. The code will more clearly define where cout and other identifiers are coming from (std::cin, std::cout). As you start using namespaces in your code it is better to identify where each function comes from because there may be function name collisions from different namespaces. The identifiercout you may override within your own classes, and you may override the operator << in your own classes as well. This stack overflow question discusses this in more detail.

Useless Comments

The comments in main() of the form //Procedure N really don't help explain anything. Comments should be used to explain why the code may be doing something that a programmer can't infer by reading self documenting code. Given the function and variable names in the program this code is self documenting code.

\$\endgroup\$
  • \$\begingroup\$ Thanks so much for reviewing my program. Can you elaborate about the return user function? I’d appreciate that greatly. I will definitely take on board about the display procedure I will fix that! Many thanks! \$\endgroup\$ – George Austin Bradley Dec 5 '19 at 21:31
  • 1
    \$\begingroup\$ To return user there should be a user class or struct. What I was describing above is that all necessary vectors should be passed into the AddUser function (note, not plural) and that the AddUser function should call each of the functions that get input. I would put off all calculations until all user information has been added. While looking into an answer for your comment I noticed that the function BillAmounts has to vectors that aren't used as vectors, this looks like a bug waiting to happen. \$\endgroup\$ – pacmaninbw Dec 6 '19 at 15:37

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.