Personal Finance Management Tool (OOP version)

Question

CodeReview Community! I was hoping you'd be able to review my basic OOP program. Last year I did a strictly procedural one and now I’ve incorporated OOP techniques. Ignore the commented out piece of code. The Hungarian Notation is a requirement for my university course so please don’t criticise the fact I’ve used it. I’d like to know what I’ve done well vs what I haven’t and any suggestions in making my code better. Many thanks in advance. The following scenario is what I’ve based the program on:

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

// Personal Finance Tool.cpp : This file contains the 'main' function. Program execution begins and ends there.
//

#include <iostream>
#include <vector>
#include <string>
#include <memory>
#include "Utils.h"
class Bill 
{
private:
    std::string m_sBillName;
    double m_dMonthlyBill;
public:
    Bill(std::string sBillName, double dMonthlyBill)
    :m_sBillName(sBillName),m_dMonthlyBill(dMonthlyBill) {}
    double GetMonthlyBill() {
        return m_dMonthlyBill;
    }
};

class User 
{
private:
    std::string m_sName;
    double m_dMonthlyWage;
    std::vector<Bill>objBillsToPay;
public:
    User(std::string sName, double dMonthlyWage)
    :m_sName(sName), m_dMonthlyWage(dMonthlyWage) {}
    
    void AddBill(std::string sBillName, double dMonthlyBill) 
    {
        objBillsToPay.push_back({ sBillName, dMonthlyBill });
    }

    double WeeklyWage() {
        return m_dMonthlyWage / 4;
    }

    double YearlyWage() {
        return m_dMonthlyWage * 12;
    }

    double TotalSpentOnBills()
    {
        double dTotal = 0;
        for (Bill &objBill : objBillsToPay) 
        {
            dTotal += objBill.GetMonthlyBill();
        }
        return dTotal;
    }

    double LeftToSaveMonthly()
    {
        return (YearlyWage() / 12) - (TotalSpentOnBills() / 12);
    }

    double TotalLeftToSaveYearly() 
    {
        return YearlyWage() - TotalSpentOnBills();
    }

    double OverSaved() 
    {
        return TotalLeftToSaveYearly() * 0.10;
    }

    double UnderSaved() {
        return TotalLeftToSaveYearly() - (TotalLeftToSaveYearly() * 0.10);
    }
};

void DisplayResults(std::vector<User>& objUsers) {
    for (User& objUser : objUsers) {
        std::cout << "***" << std::endl;
        std::cout << "Weekly wage: " << objUser.WeeklyWage() << std::endl;
        std::cout << "Yearly wage: " << objUser.YearlyWage() << std::endl;
        std::cout << "10% over total that can be saved: " << objUser.OverSaved() << std::endl;
        std::cout << "10% under total that can be saved: " << objUser.UnderSaved() << std::endl;
        std::cout << "Total left to save yearly: " << objUser.TotalLeftToSaveYearly() << std::endl;
        std::cout << "Total spent on bills: " << objUser.TotalSpentOnBills() << std::endl;  
    }
    std::cout << std::endl;
}

void TestData(std::vector<User>& objUsers)
{
    objUsers.push_back({ "Jack Kimmins", 764});
    objUsers.at(0).AddBill("Water Bill", 65);
    objUsers.push_back({ "George Bradley", 332});
    objUsers.push_back({ "Jason Hill", 343 });
    objUsers.push_back({ "Sean Shearing", 374 });
}

int main()
{
    std::vector<User>objUsers;
    TestData(objUsers);
    //CreateUser(objUsers); Will need to be wrapped in a while loop if uncommented
    DisplayResults(objUsers);
    system("pause"); //Ignore this, I know using system isn't good, just to stop it
}

Answer 1

Welcome to the CR community!

Avoid making copies for non-builtin types

As a good rule of thumb, pass large objects that you don't need to modify by a constant reference.
This will make sure that you don't create extra copies. However, you don't need to do this for the built-in types like int, float, char, double...

If you have a variable x. Passing it by constant reference means const type& x. What you do here is basically pass the memory address, which gets copied into x. This way you aren't passing the whole string class. But just an address in the memory, which points to an existing string class.

If you wanted something to get delivered to your house, would you give the company your address or your house?

Why const? This makes sure that you don't accidentally modify the value of x. Because if you do, the value will change everywhere.

everywhere? I mean the value you used to call the function.

void fun(int& x)
{
    x = 5;
}

int x = 10;
fun(); // x is now 5!

Using const will tell the compiler, "If you see me modifying this, say something so I can avoid a nightmare ".

Prefer using '\n' over std::endl

std::endl calls std::flush everytime. This makes it less efficient than printing '\n'. They both will achieve the job here, except one will be faster.

For example

std::cout << "***" << std::endl;

This calls the << operator twice + calls std::flush. The same newline can be achieved with

std::cout << "***\n";

This will be more efficient

Maintain consistent formatting

Your code has some inconsistent formatting. Your IDE should easily be able to format code into different styles, or even a site like this one can do the job.

---

Re-format code into different files

As your code grows, you will realize that you cannot have all your code in main.cpp. Doing that makes your code just less readable. You will find it tougher to maintain your code later on too. What if you just had Bill.h and User.h! This way every time you have a problem, you can easily navigate to these files. Moreover, your code looks much cleaner

re-formatted into separate files

Bill.h

#include <string>

class Bill 
{
    private:
        std::string m_sBillName;
        double m_dMonthlyBill;
        
    public:
        Bill(std::string,double);
        double GetMonthlyBill();
};

Bill.cpp

#include "Bill.h"


Bill::Bill(std::string sBillName, double dMonthlyBill)
    :m_sBillName(sBillName),m_dMonthlyBill(dMonthlyBill) 
    
    {}

double Bill::GetMonthlyBill() {
    return m_dMonthlyBill;
}

---

User.h

#include <string>
#include <vector>
#include "Bill.h"

class User 
{
    private:
        std::string m_sName;
        double m_dMonthlyWage;
        std::vector <Bill> objBillsToPay;
        
    public:
        User(std::string,double);
        void AddBill(std::string,double);
        double WeeklyWage();
        double YearlyWage();
        double TotalSpentOnBills();
        double LeftToSaveMonthly();
        double TotalLeftToSaveYearly();
        double OverSaved();
        double UnderSaved();
    
};

User.cpp

#include "User.h"



User::User(std::string sName, double dMonthlyWage)
        :m_sName(sName), m_dMonthlyWage(dMonthlyWage) 
    {}
    
void User::AddBill(std::string sBillName, double dMonthlyBill) 
{
    objBillsToPay.push_back({ sBillName, dMonthlyBill });
}

double User::WeeklyWage() {
    return m_dMonthlyWage / 4;
}

double User::YearlyWage() {
    return m_dMonthlyWage * 12;
}

double User::TotalSpentOnBills()
{
    double dTotal = 0;
    for (Bill &objBill : objBillsToPay) 
    {
        dTotal += objBill.GetMonthlyBill();
    }
    return dTotal;
}

double User::LeftToSaveMonthly()
{
    return (YearlyWage() / 12) - (TotalSpentOnBills() / 12);
}

double User::TotalLeftToSaveYearly() 
{
    return YearlyWage() - TotalSpentOnBills();
}

double User::OverSaved() 
{
    return TotalLeftToSaveYearly() * 0.10;
}

double User::UnderSaved() {
    return TotalLeftToSaveYearly() - (TotalLeftToSaveYearly() * 0.10);
}

---

• What is std::string m_sBillName; doing in Bill? You have declared it private but you aren't using it in any of the member functions, and since it is private it won't be accessible outside either.

Answer 2

Avoid Hungarian notation

You should avoid using Hungarian notation, especially if you are going to prefix everything that is not a primitive type with obj. It becomes really useless at that point. You also fail to distinguish between objects passed by value and by reference.

Using the m_ prefix to mark member variables is something that is done correctly though, so no problem in keeping that.

Why does a Bill have a name?

You give Bills names, but you can never read back the name. It is useless information at that point. If you don't store the name, then the only thing left is the variable dMonthlyBill, which is just a double. If that's all there is to it, I would say that this class doesn't do anything usefull, and should be removed. Instead, in class User you can write:

std::vector<double> monthly_bills;

But, the same goes for class User. There's no way to get the name or the list of bills out of it. The only thing it really needs to store is the monthly wage and the sum of the monthly bills:

class User
{
    double m_monthly_wage;
    double m_monthly_bills = 0;

public:
    User(double monthly_wage): m_monthly_wage(monthly_wage) {}

    void AddBill(double monthly_bill) {
        m_monthly_bills += monthly_bill;
    }

    ...
}

Be careful converting between time periods

You cannot blindly convert monthly wages to weekly wages by dividing by four. After all, a year has 52.1775 weeks, not 48!

Furthermore, the calculation in LeftToSaveMonthly() is wrong: TotalSpentOnBills() is already per month, so you shouldn't divide it by 12. And there is no need to call YearlyWage() and then divide the answer by 12 again. You can just write:

double LeftToSaveMonthly()
{
    return m_dMonthlyWage - TotalSpentOnBills();
}

Avoid using system()

Don't use system() unless you really need it, it is very inefficient and not portable. In this case, you can simply write:

std::cin.get();

This might require the user to press Enter though instead of any key (if the input is line-buffered), but that should be fine.

You already write in the comments in the code that you know it is bad, but why do it anyway then?

Answer 3

Instead of DisplayResults, opt to overload the << operator for your User class

std::ostream& operator<<(std::ostream& os, User& const u) {
    os << "***" << '\n';
    os << "Weekly wage: " << u.WeeklyWage() << '\n';
    os << "Yearly wage: " << u.YearlyWage() << '\n';
    os << "10% over total that can be saved: " << u.OverSaved() << '\n';
    os << "10% under total that can be saved: " << u.UnderSaved() << '\n';
    os << "Total left to save yearly: " u.TotalLeftToSaveYearly() << '\n';
    os << "Total spent on bills: " << u.TotalSpentOnBills() << '\n';
    return os;
}

Advantages:

• No longer beholden to outputting only through std::cout, can output to file instead.
• Easy to read
• Enables outputting a single or multiple users
• Can now use the std::copy and std::copy_if-to-output-iterator if you want

Don't forget to add friend ostream& operator<<(ostream& os, User const& u); to your class!

You can also do this for the Bill class.

Additionally, after you split out the users

double User::TotalSpentOnBills()
{
    return std::accumulate(
        objBillsToPay.cbegin(),
        objBillsToPay.cend(),
        0.0,
        [](auto const & accumulator, auto const & item){
            return accumulator + item;
        });
}