4
\$\begingroup\$

I was wondering if you'd be able to review my basic Tax Calculator that incorporates OOP techniques. I'd greatly appreciate it if you didn't mention about my variable naming convention i.e. Hungarian notation, it is a requirement for my university course. Also, please ignore the fact it says about "Windows Forms Application", this is purely a console app in C++. Furthermore, please just treat my program as it is supposed to be inputted I haven't done error-checking. The main point of this is just to get feedback on how I've used OOP techniques. Thanks very much in advance.

enter image description here

// Tax Calculator.cpp : This file contains the 'main' function. Program execution begins and ends there.
#include <iostream>
#include <vector>
#include <string>
#include <iomanip>
using namespace std;

class Person {
private:
    string m_sName;
    string m_sBandIncomeTaxName;
    
    bool m_bPayIncomeTax;
    bool m_bPayStudentLoan;

    double m_dAnnualSalary;
    double m_dTaxRate;
    double m_dMonthlyIncomeTax;
    double m_dWeeklySalary;
    double m_dNationalInsurancePay;
    double m_dNationalInsuranceRate;
    double m_dStudentLoanPay;
    double m_dTakeHomePay;
  
public:

    //Constructor
    Person() :
        m_sName("No name specified"), 
        m_sBandIncomeTaxName("No name specified"), 
        m_bPayStudentLoan(0.0),
        m_bPayIncomeTax(0.0), m_dAnnualSalary(0.0), 
        m_dTaxRate(0.0), 
        m_dMonthlyIncomeTax(0.0),
        m_dWeeklySalary(0.0), 
        m_dNationalInsurancePay(0.0), 
        m_dNationalInsuranceRate(0.0),
        m_dStudentLoanPay(0.0), 
        m_dTakeHomePay(0.0){}

    string GetName() const { return m_sName; }
    double GetNationalInsuranceRate() { return m_dNationalInsuranceRate; }
    double GetNationalInsurancePay() { return m_dNationalInsurancePay; }
    double GetWeeklySalary() { return m_dWeeklySalary; }
    double GetAnnualSalary() const { return m_dAnnualSalary; }
    double GetMonthlyIncomeTax() { return m_dMonthlyIncomeTax; }
    double GetStudentLoanPay() { return m_dStudentLoanPay; }
    double GetMonthlyTakeHomePay() { return m_dTakeHomePay; }
    bool GetStudentLoanBool() { return m_bPayStudentLoan; }
    bool GetIncomeTaxBool() { return m_bPayIncomeTax; }

    void SetNationalInsuranceRate(double dNationalInsuranceRate) { m_dNationalInsuranceRate = dNationalInsuranceRate; };
    void SetName(string sName) { m_sName = sName; }
    void SetAnnualSalary(double dAnnualSalary) { m_dAnnualSalary = dAnnualSalary; }
    void SetPayIncomeTax(bool bPayIncomeTax) { m_bPayIncomeTax = bPayIncomeTax; }
    void SetPayStudentLoan(bool bPayStudentLoan) { m_bPayIncomeTax = bPayStudentLoan; }
    void SetBandIncomeTaxName(string sBandIncomeTaxName) { m_sBandIncomeTaxName = sBandIncomeTaxName; }
    void SetTaxRate(double dTaxRate) { m_dTaxRate = dTaxRate; }

    void CalculateMonthlyIncomeTax();
    void CalculateWeeklyIncome();
    void CalculateNationalInsurance();
    void CalculateAnnualStudentLoan();
    void CalculateMonthlyTakeHomePay();
};

void Person::CalculateMonthlyIncomeTax() { 
    m_dMonthlyIncomeTax = (m_dAnnualSalary * m_dTaxRate) / 12; 
}
void Person::CalculateWeeklyIncome() {
    m_dWeeklySalary = (m_dAnnualSalary / 12) / 4; 
}
void Person::CalculateNationalInsurance() {
    double dRemainderOver = 0;
    if (GetWeeklySalary() > 162 && GetWeeklySalary() < 892) {

        dRemainderOver = GetWeeklySalary() - 162;
        SetNationalInsuranceRate(0.12);
        m_dNationalInsurancePay = dRemainderOver * GetNationalInsuranceRate();
    }
    else
    {
        dRemainderOver = GetWeeklySalary() - 892;
        SetNationalInsuranceRate(0.2);
        m_dNationalInsurancePay = dRemainderOver * GetNationalInsuranceRate();
    }
}
void Person::CalculateAnnualStudentLoan() {
    if (m_dAnnualSalary > 25000) {
        m_dStudentLoanPay = m_dAnnualSalary * 0.9;
    }
    else {
        m_dStudentLoanPay = 0;
    }
}
void Person::CalculateMonthlyTakeHomePay() { 
    m_dTakeHomePay = (m_dAnnualSalary / 12) - (m_dNationalInsurancePay * 4) - (m_dStudentLoanPay / 12) - m_dMonthlyIncomeTax; 
}

void AddPerson(Person& objPerson)
{
    std::string sName = "";
    double dAnnualSalary = 0;
    std::cout << "Enter the name of the person: " ;
    cin >> sName;
    objPerson.SetName(sName);

    std::cout << "How much do you get annually?: " << char(156);
    cin >> dAnnualSalary;
    objPerson.SetAnnualSalary(dAnnualSalary);
}

void DoYouPayAnnualIncomeTax(Person& objPerson)
{
    unsigned char cChoice;
    bool bPayAnnualIncomeTax = false;
    bool bPayStudentLoan = false;

    std::cout << "Do you pay annual income tax?: (y/n) ";
    cin >> cChoice;
    cChoice = tolower(cChoice);
    if (cChoice == 'y')
    {
        objPerson.SetPayIncomeTax(true);
    }
    else if ('n') {
        objPerson.SetPayIncomeTax(false);
    }
    else
    {
        std::cout << "You've entered an invalid choice!\n";
    }
}
void DoYouPayStudentLoan(Person& objPerson)
{
    unsigned char cChoice;
    bool bPayAnnualIncomeTax = false;
    bool bPayStudentLoan = false;

    std::cout << "Do you pay a student loan (y/n): ";
    cin >> cChoice;
    cChoice = tolower(cChoice);
    if (cChoice == 'y')
    {
        objPerson.SetPayStudentLoan(true);
    }
    else if ('n') {
        objPerson.SetPayStudentLoan(false);
    }
    else
    {
        std::cout << "You've entered an invalid choice!\n";
    }
}

void CalcBandIncomeTaxPayer(Person& objPerson) {

    if (objPerson.GetAnnualSalary() < 11850)
    {
        objPerson.SetBandIncomeTaxName("Personal Allowance");
        objPerson.SetTaxRate(0.0);

    }
    else if (objPerson.GetAnnualSalary() < 46350)
    {
        objPerson.SetBandIncomeTaxName("Basic Rate");
        objPerson.SetTaxRate(0.2);
    }
    else if (objPerson.GetAnnualSalary() < 150000)
    {
        objPerson.SetBandIncomeTaxName("Higher Rate");
        objPerson.SetTaxRate(0.4);
    }
    else
    {
        objPerson.SetBandIncomeTaxName("Additional Rate");
        objPerson.SetTaxRate(0.45);
    }

}

void DisplayResults(Person &objPerson) {
    std::cout << "*********************" << std::endl;
    if (objPerson.GetIncomeTaxBool() == true) {
        objPerson.CalculateMonthlyIncomeTax();
        std::cout << "You pay in monthly income tax: " << char(156) << objPerson.GetMonthlyIncomeTax() << std::endl;
    }
    if (objPerson.GetStudentLoanBool()) 
    {
        objPerson.CalculateAnnualStudentLoan();
        std::cout << "Annual Student loan: " << char(156) << objPerson.GetStudentLoanPay() << std::endl;
    }
    //Make the calculations
    std::cout << "Monthly National Insurance: " << char(156) << objPerson.GetNationalInsurancePay() << std::endl;
    std::cout << "Monthly Student Loan: " << char(156) << objPerson.GetStudentLoanPay() << std::endl;
    std::cout << "Take home pay: " << char(156) << objPerson.GetMonthlyTakeHomePay();
    std::cout << "*********************" << std::endl;
}

int main()
{
    std::cout << std::setprecision(2) << std::fixed;
    Person objPerson;
    //Get input from user
    AddPerson(objPerson);
    DoYouPayAnnualIncomeTax(objPerson);
    DoYouPayStudentLoan(objPerson);
    CalcBandIncomeTaxPayer(objPerson);
    objPerson.CalculateWeeklyIncome();
    objPerson.CalculateNationalInsurance();
    objPerson.CalculateMonthlyTakeHomePay();
    DisplayResults(objPerson);
}
\$\endgroup\$
5
  • \$\begingroup\$ my variable naming convention is a requirement for my university course. Great. That said, you won't spend the rest of your career in this university course - you'll spend it in industry, whose standards are somewhat different from company to company. All constructive feedback is on topic, and it's important that you get a feeling for what the rest of the universe understands to be standard. \$\endgroup\$
    – Reinderien
    Nov 13, 2020 at 15:33
  • \$\begingroup\$ For C++ there aren't very strong naming standards, mind you, so Hungarian notation is fine. \$\endgroup\$
    – Reinderien
    Nov 13, 2020 at 15:34
  • \$\begingroup\$ @Reinderien That's right, the win32 API also uses the Hungarian notation \$\endgroup\$
    – user228914
    Nov 13, 2020 at 16:13
  • 4
    \$\begingroup\$ It's good to include the problem statement, but please not as an image! Also, ensure you have the legal right to reproduce that if you didn't write it yourself. It's normally better to summarise the problem in your own words. \$\endgroup\$ Nov 13, 2020 at 20:52
  • 1
    \$\begingroup\$ I'm a big fan of Hungarian notation. However, you are doing Hungarian notation wrong! Simonyi didn't mean to repeat the actual type in the prefix; he meant the semantic type. So, d is useless as a prefix because the variable is already a double. You and the compiler both know that already. Correct Hungarian notation would be something like c to indicate that the double is semantically a currency value. This is something neither you or the compiler know. Other examples include things like u to mean "unsafe"/"unsanitized"/"user input", and sz to mean NUL-terminated string. \$\endgroup\$ Nov 14, 2020 at 5:02

4 Answers 4

5
\$\begingroup\$

Overall Observations

The program would be simpler if the following functions were members of the Person Class:

  1. void AddPerson(Person& objPerson)
  2. void DoYouPayAnnualIncomeTax(Person& objPerson)
  3. void DoYouPayStudentLoan(Person& objPerson)
  4. void CalcBandIncomeTaxPayer(Person& objPerson)
  5. void DisplayResults(Person& objPerson)

In that case the class Person would not need most of the getter and setter functions, and all or most of the attributes / member variables could be private. As members of Person those functions would have direct access to private variables and could skip some intermediate steps.

Example:

void Person::CalcBandIncomeTaxPayer() {

    if (m_dAnnualSalary < 11850)
    {
        m_sBandIncomeTaxName = "Personal Allowance";
        m_dTaxRate = 0.0;

    }
    else if (m_dAnnualSalary < 46350)
    {
        m_sBandIncomeTaxName = "Basic Rate";
        m_dTaxRate = 0.2;
    }
    else if (m_dAnnualSalary < 150000)
    {
        m_sBandIncomeTaxName = "Higher Rate";
        m_dTaxRate = 0.4;
    }
    else
    {
        m_sBandIncomeTaxName = "Additional Rate";
        m_dTaxRate = 0.45;
    }

}

Addperson should call void DoYouPayAnnualIncomeTax(Person& objPerson) and void DoYouPayStudentLoan(Person& objPerson).

Avoid using namespace std;

The code is inconsistently using std:: there are functions where std::cout is used and then the next line uses cin. Whatever you do, be consistent.

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.

Magic Numbers

There are Magic Numbers throughout the program, in the CalcBandIncomeTaxPayer() function examples would be: 11850, 0.0, 46350 and 0.2; it would better to create symbolic constants for them to make the code more readable and easier to maintain. These numbers may be used in many places and being able to change them by editing only one line makes maintenance easier.

Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. There is a discussion of this on stackoverflow.

\$\endgroup\$
8
\$\begingroup\$

using namespace std

It is bad practice to have this in your program. Sometimes it is OK if you are writing a small program for something like a programming challenge. Otherwise, I highly discourage the use of this in your programs. The std namespace is HUGE and writing this statement means that now you have no idea what is a part of the standard library and what isn't, you have just erased that line.


Default member initialization

    string m_sName;
    string m_sBandIncomeTaxName;

    bool m_bPayIncomeTax;
    bool m_bPayStudentLoan;

    double m_dAnnualSalary;
    double m_dTaxRate;
    double m_dMonthlyIncomeTax;
    double m_dWeeklySalary;
    double m_dNationalInsurancePay;
    double m_dNationalInsuranceRate;
    double m_dStudentLoanPay;
    double m_dTakeHomePay;

public:

    //Constructor
    Person() :
        m_sName("No name specified"),
        m_sBandIncomeTaxName("No name specified"),
        m_bPayStudentLoan(0.0),
        m_bPayIncomeTax(0.0), m_dAnnualSalary(0.0),
        m_dTaxRate(0.0),
        m_dMonthlyIncomeTax(0.0),
        m_dWeeklySalary(0.0),
        m_dNationalInsurancePay(0.0),
        m_dNationalInsuranceRate(0.0),
        m_dStudentLoanPay(0.0),
        m_dTakeHomePay(0.0) {}

I feel you can use default member initialization here since it will do the same job but look a little cleaner since you will avoid repeating the name. This thread on Satck Overflow talks about it in detail

    private:
        string m_sName{ "No name specified" };
        string m_sBandIncomeTaxName{ "No name specified" };

        bool m_bPayIncomeTax = false;
        bool m_bPayStudentLoan = false;

        double m_dAnnualSalary = 0;
        double m_dTaxRate = 0;
        double m_dMonthlyIncomeTax = 0;
        double m_dWeeklySalary = 0;
        double m_dNationalInsurancePay = 0;
        double m_dNationalInsuranceRate = 0;
        double m_dStudentLoanPay = 0;
        double m_dTakeHomePay = 0;

Avoid using getters and setters

    string GetName() const { return m_sName; }
    double GetNationalInsuranceRate() { return m_dNationalInsuranceRate; }
    double GetNationalInsurancePay() { return m_dNationalInsurancePay; }
    double GetWeeklySalary() { return m_dWeeklySalary; }
    double GetAnnualSalary() const { return m_dAnnualSalary; }
    double GetMonthlyIncomeTax() { return m_dMonthlyIncomeTax; }
    double GetStudentLoanPay() { return m_dStudentLoanPay; }
    double GetMonthlyTakeHomePay() { return m_dTakeHomePay; }
    bool GetStudentLoanBool() { return m_bPayStudentLoan; }
    bool GetIncomeTaxBool() { return m_bPayIncomeTax; }

    void SetNationalInsuranceRate(double dNationalInsuranceRate) { m_dNationalInsuranceRate = dNationalInsuranceRate; };
    void SetName(string sName) { m_sName = sName; }
    void SetAnnualSalary(double dAnnualSalary) { m_dAnnualSalary = dAnnualSalary; }
    void SetPayIncomeTax(bool bPayIncomeTax) { m_bPayIncomeTax = bPayIncomeTax; }
    void SetPayStudentLoan(bool bPayStudentLoan) { m_bPayIncomeTax = bPayStudentLoan; }
    void SetBandIncomeTaxName(string sBandIncomeTaxName) { m_sBandIncomeTaxName = sBandIncomeTaxName; }
    void SetTaxRate(double dTaxRate) { m_dTaxRate = dTaxRate; }

If you simply make the necessary variables public, you will remove the need to have so many extra functions, which would make your code magically look cleaner, even though you did nothing.


Taking input that has whitespaces

    std::string sName = "";
    double dAnnualSalary = 0;
    std::cout << "Enter the name of the person: " ;
    cin >> sName;

There is a problem here, to check it lets test it with some input

Enter the name of the person: Aryan Parekh
How much do you get annually?: £Do you pay annual income tax?: (y/n) Do you pay a student loan (y/n): *********************
Monthly National Insurance: £-178.40
Monthly Student Loan: £0.00
Take home pay: £713.60*********************

As you can see, immediately after I entered my name the program starts doing weird things. The reason is std::cin will stop reading when it finds a whitespace.

Aryan Parekh 
     ^
     It stops here

But Parekh is still there, so for the next prompt which is How much do you get annually?, it takes Parekh when it's expecting a double. After that it goes all downhill.
Unless the users are only going to have one word in their name, you will have to use std::getline

std::getline( std::cin, sName );

Now when we test with the same input

Enter the name of the person: Aryan Parekh
How much do you get annually?: £

That works.


Magic numbers

There are magic numbers throughout your program, unnamed numbers. You should name them. Because it is impossible for others who are reading your code to find out what they mean unless you already know what a line does are you explicitly mention it.

    if (GetWeeklySalary() > 162 && GetWeeklySalary() < 892) {

        dRemainderOver = GetWeeklySalary() - 162;
        SetNationalInsuranceRate(0.12);
        m_dNationalInsurancePay = dRemainderOver * GetNationalInsuranceRate();
    }

Always validate your input

There are many instances in your program where you haven't checked whether the user had entered what was asked for. For example

std::cout << "Do you pay a student loan (y/n): ";
cin >> cChoice;

The user can always accidentally enter something else or even nothing at all. You always should validate your input. Otherwise, you're counting on the user to enter everything perfectly because if he doesn't your program will go bonkers.

The question also states that your programme should not accept negative/0 as salary input, but you haven't handled that.

\$\endgroup\$
8
  • \$\begingroup\$ The original poster clearly states at the beginning of the question that they are not checking input, that puts a little restriction on what we can review. \$\endgroup\$
    – pacmaninbw
    Nov 13, 2020 at 17:25
  • \$\begingroup\$ Also limit your answers on homework questions. \$\endgroup\$
    – pacmaninbw
    Nov 13, 2020 at 17:29
  • \$\begingroup\$ @pacmaninbw I did read that, but I wasn't sure as to what he meant by " Furthermore, please just treat my program as it is supposed to be inputted I haven't done error-checking " \$\endgroup\$
    – user228914
    Nov 13, 2020 at 17:30
  • \$\begingroup\$ @pacmaninbw What do you mean? \$\endgroup\$
    – user228914
    Nov 13, 2020 at 17:30
  • 2
    \$\begingroup\$ I remember being taught, "don't treat your user like an input file". \$\endgroup\$
    – JDługosz
    Nov 13, 2020 at 22:16
4
\$\begingroup\$

Others have mentioned the scattering of magic numbers through the code, and suggested that you give names to them. I'd consider going further than that, and make the tax rules as much as possible be data-driven. In the Real World, governments tweak the thresholds pretty frequently, and you wouldn't want to have to re-compile versions of your program with different constants embedded in it every tax year.

As a starter, you could define

std::map<int,int> tax_rates = {
    // upper limit (GBP) => rate (percent)
    {11850, 0},
    {46350, 20},

And then when government adds a new 50% band, or some of your people are on the Scottish scale, you can apply those changes without changing the logic.

Going further, it would be better to be able to load the rates from data files.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ The one problem I see with this is that the string for tax band name doesn't get assigned, but I don't know if that is important or not. \$\endgroup\$
    – pacmaninbw
    Nov 14, 2020 at 12:32
  • 1
    \$\begingroup\$ If that's important, then it would be worth defining a struct tax_band with everything that's needed. Left as an exercise for the interested reader... \$\endgroup\$ Nov 15, 2020 at 10:27
4
\$\begingroup\$

Personally, I would suggest that you should have two classes, not just one and some static functions.

Consider the extensibility of this for the future.

If you have class person and another class taxregime then you have the makings of useful program. As someone else has said, all the constants that you use should be configurable. By having the taxregime class these values could get injected in the constructor for that class. As such, you could now create tax2020, tax2019, tax2018, etc as instances of that class.

If that taxregime class then has the fundamental calculations in, then you pass it the instance of the person so

     thisYear.getAnnualTax(thisPerson)

Moreover, when the actual tax regime in the country changes, to include extra tax bands etc, or withdrawing personal allowances once you exceed an income threshold, the calculation logic will need to change.

Your program would become extensible by deriving a new class taxRegime2022 from taxRegime and the specific logic that you need to override, for NI, or for tax, can be overridden in there.

Now you have something where you can make projections or reports based on both the person and the year of income. (If you earned X in 2018 you'd pay this, but if you earned it in 2022 you'd pay this other amount)

That goes beyond what the question specifically asks for, but solves the real world problem, rather than just the simple requirements.

Only you can work out whether there is value in doing that for this assignment, or in a job.

EDIT: pacmaninbow's useful answer suggested a Person::CalcBandIncomeTaxPayer() method. It's a great idea BUT...

From a data point of view, the magic numbers and the calculation have nothing to do with a person, which is why they shouldn't be there. They have to do with the tax regime, and that's why I think you need that second class. Object orientation is primarily about encapsulating and isolating data correctly.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.