Sales tax calculator

Question

Please review the sales tax problem which has been designed using strategy pattern.

The Problem:

Basic sales tax is applicable at a rate of 10% on all goods, except books, food, and medical products that are exempt. Import duty is an additional sales tax applicable on all imported goods at a rate of 5%, with no exemptions. Also the Sales Tax should be rounded off to the nearest 0.05.

Item.h

/*
 * Item.h
 *
 *  Created on: Jun 7, 2011
 *      Author: som
 */

#ifndef ITEM_H_
#define ITEM_H_

class SalesTax;

//This represents the Items which don't have an Import duty or any sales tax
class Item
{
public:

//Constructors
    Item();
    Item(SalesTax *aSalesTax);

//Interface Functions for Item

//To calculate the price after tax and import duty
    virtual void CalculateTotalPrice();

//To calculate the total tax and import duty
    virtual void CalculateTotalTax();

//To set the price of the Items
    void SetPrice(double aPrice);

//To get the price of the Items before tax
    double getPrice();

//To get the price of the items after tax
    double getTotalPrice();

//To get the total tax and import duty of the items
    double getTax();

//Data
protected:
//Works as the Strategy of the Sales Tax problem.
//If in future the tax calculation becomes more complicated for different Items
//we will just have to change this Strategy. We can also subclass this Strategy class
//for future expansion of the tax calculation strategy
    SalesTax *iSalesTax;
//Data
protected:

//These are the basic properties of any Item.
//Hence these are made protected members so that the subclasses of Item can inherit
//these properties
    double iPrice;
    double iTotalPrice;
    double iTotalTax;
};

//This class represents the Items which have only Import Duty
class ImportedItem : virtual public Item
{
public:
//Constructors
    ImportedItem();

//This constructor helps to create Items having only Import duty
    ImportedItem(SalesTax *aSalesTax, double aImportDuty);

//Override
    virtual void CalculateTotalTax();

protected:
    double iImportDuty;
};

//This class represents the Items which have only Sales Tax but no Import Duty
class NonFoodBookMedicalItem : virtual public Item
{
public:
//Constructors
    NonFoodBookMedicalItem();

//This constructor helps to create Items having only Sales tax
    NonFoodBookMedicalItem(SalesTax *aSalesTax, double aRate);
//Override
    virtual void CalculateTotalTax();

protected:
    double iRate;
};

//This class represents the Items which have got both Import Duty as well as sales Tax
class NormalItem: public ImportedItem, public NonFoodBookMedicalItem
{
public:
    NormalItem();
//This constructor helps to create Items having both Sales tax and Import duty
    NormalItem(SalesTax *aSalesTax, double aRate, double aImportDuty);

//Override
    virtual void CalculateTotalTax();
};

#endif /* ITEM_H_ */

Item.cpp

/*
 * Item.cpp
 *
 *  Created on: Jun 7, 2011
 *      Author: som
 */

#include "SalesTax.h"
#include "Item.h"

Item::Item() {}

Item::Item(SalesTax *aSalesTax): iSalesTax(aSalesTax), iPrice(0), iTotalPrice(0), iTotalTax(0)
{
}

void Item::CalculateTotalPrice()
{
    iTotalPrice = iPrice + iTotalTax;
}

double Item::getTotalPrice()
{
    return iTotalPrice;
}


void Item::CalculateTotalTax()
{
    iTotalTax = iSalesTax->ComputeSalesTax(iPrice, 0, 0);
}

void Item::SetPrice(double aPrice)
{
    iPrice = aPrice;
}

double Item::getPrice()
{
    return iPrice;
}

double Item::getTax()
{
    return iTotalTax;
}
ImportedItem::ImportedItem() {}

ImportedItem::ImportedItem(SalesTax *aSalesTax, double aImportDuty): Item(aSalesTax)
{
    iImportDuty = aImportDuty;
}
void ImportedItem::CalculateTotalTax()
{
    iTotalTax = iSalesTax->ComputeSalesTax(iPrice, 0, iImportDuty);

}
NonFoodBookMedicalItem::NonFoodBookMedicalItem() {}

NonFoodBookMedicalItem::NonFoodBookMedicalItem(SalesTax *aSalesTax, double aRate): Item(aSalesTax)
{
    iRate = aRate;
}

void NonFoodBookMedicalItem::CalculateTotalTax()
{
    iTotalTax = iSalesTax->ComputeSalesTax(iPrice, iRate, 0);

}
NormalItem::NormalItem() {}

NormalItem::NormalItem(SalesTax *aSalesTax, double aRate, double aImportDuty): Item(aSalesTax)
{
    iRate = aRate;
    iImportDuty = aImportDuty;
}
void NormalItem::CalculateTotalTax()
{
    iTotalTax = iSalesTax->ComputeSalesTax(iPrice, iRate, iImportDuty);
}

ItemCreator.h

/*
 * ItemCreator.h
 *
 *  Created on: Jun 7, 2011
 *      Author: som
 */

#ifndef ITEMCREATOR_H_
#define ITEMCREATOR_H_
#include "Item.h"

const int ITEM_WITH_NOSALESTAX_AND_IMPORTDUTY = 1;
const int ITEM_WITH_NOSALESTAX_ONLY_IMPORTDUTY = 2;
const int ITEM_WITH_ONLY_SALESTAX_AND_NOIMPORTDUTY = 3;
const int ITEM_WITH_BOTH_SALESTAX_AND_IMPORTDUTY = 4;

const double SALES_TAX_RATE = 10;
const double IMPORT_DUTY_RATE = 5;

class Not_A_Standard_Item_Type_Exception
{
public:
    void printerrormsg();
};
class ItemCreator
{
public:
    virtual Item *Create(int aItemId);

};

#endif /* ITEMCREATOR_H_ */

ItemCreator.cpp

/*
 * ItemCreator.cpp
 *
 *  Created on: Jun 7, 2011
 *      Author: som
 */

#include "ItemCreator.h"
#include "Item.h"
#include "SalesTax.h"

#include <iostream>

using namespace std;

void Not_A_Standard_Item_Type_Exception::printerrormsg()
{
    cout << "Not the right Item Type..." << endl;
}

Item *ItemCreator::Create(int aItemId)
{
    SalesTax *st = new SalesTax();

    switch(aItemId)
    {
        case ITEM_WITH_NOSALESTAX_AND_IMPORTDUTY:
            return new Item(st);
            break;

        case ITEM_WITH_NOSALESTAX_ONLY_IMPORTDUTY:
            return new ImportedItem(st, IMPORT_DUTY_RATE);
            break;

        case ITEM_WITH_ONLY_SALESTAX_AND_NOIMPORTDUTY:
            return new NonFoodBookMedicalItem(st, SALES_TAX_RATE);
            break;

        case ITEM_WITH_BOTH_SALESTAX_AND_IMPORTDUTY:
            return new NormalItem(st, SALES_TAX_RATE, IMPORT_DUTY_RATE);
            break;

        default:
            throw Not_A_Standard_Item_Type_Exception();
    }
}

SalesTax.h

/*
 * SalesTax.h
 *
 *  Created on: Jun 7, 2011
 *      Author: som
 */

#ifndef SALESTAX_H_
#define SALESTAX_H_
//This class works as the Strategy of the Sales tax problem
class SalesTax
{
public:

//Default constructor
    SalesTax();

//This function helps to compute the Sales Tax
    virtual double ComputeSalesTax(double aPrice, double aRate, double aImportduty);

private:
//This is an helper function which will round off the sales tax
    double RoundOff(double aTax);
};

#endif /* SALESTAX_H_ */

SalesTax.cpp

/*
 * SalesTax.cpp
 *
 *  Created on: Jun 7, 2011
 *      Author: som
 */
#include "SalesTax.h"

SalesTax::SalesTax() {}
double SalesTax::ComputeSalesTax(double aPrice, double aRate, double aImportduty)
{
    double tx = (aPrice * aRate / (double(100))) + (aPrice * aImportduty / (double(100)));
    return RoundOff(tx);
}
//private:
double SalesTax::RoundOff(double aTax)
{
    int taxTemp = (int)aTax;
    double decimaltaxTemp = (double)(aTax - (int)taxTemp);
    int tempy = (int)(1000 * decimaltaxTemp) / 100;
    int tempz = (int)(1000 * decimaltaxTemp - tempy * 100);
    int temp = (int)(tempz / 10);
    int t = tempz % 10;

    if(t >= 5)
        temp += 1;

    return (double)(taxTemp + tempy * (0.1) + temp * (0.01));
}

main.cpp

/*
 * main.cpp
 *
 *  Created on: Jun 7, 2011
 *      Author: som
 */

#include "SalesTax.h"
#include "Item.h"
#include "ItemCreator.h"

#include <iostream>
#include <vector>

using namespace std;

int main()
{
    typedef vector<Item *> listOfItem;
    listOfItem::iterator theIterator;

    listOfItem Basket;
    char answer = 'n';

    double totalprice = 0;
    double totaltax = 0;

    do
    {
        int type_of_item;
        cout << "Enter the type of Item...1,2,3,4" << endl;
        cout << "1 for ITEM_WITH_NOSALESTAX_AND_NOIMPORTDUTY" << endl;
        cout << "2 for ITEM_WITH_NOSALESTAX_ONLY_IMPORTDUTY" << endl;
        cout << "3 for ITEM_WITH_ONLY_SALESTAX_AND_NOIMPORTDUTY" << endl;
        cout << "4 for ITEM_WITH_BOTH_SALESTAX_AND_IMPORTDUTY" << endl;
        cin >> type_of_item;
        ItemCreator *itemCreator = new ItemCreator();
        try
        {
            Item *item = itemCreator->Create(type_of_item);
            cout << "Enter the price of the Item" << endl;
            double price;
            cin >> price;
            item->SetPrice(price);
            Basket.push_back(item);
        }

        catch(Not_A_Standard_Item_Type_Exception &e)
        {
            e.printerrormsg();
        }

        cout << "Do you want to continue... Y/N" << endl;
        cin >> answer;
    }
    while(answer == 'y');
    theIterator = Basket.begin();
    int pos = 0;
    while(theIterator != Basket.end())
    {
        Basket.at(pos)->CalculateTotalTax();
        totaltax += Basket.at(pos)->getTax();
        Basket.at(pos)->CalculateTotalPrice();
        double price = Basket.at(pos)->getPrice();
        double price_after_tax = Basket.at(pos)->getTotalPrice();
        totalprice += price_after_tax;
        cout << "Item" << pos + 1 << " price " << price << endl;
        theIterator++;
        pos++;
    }
    cout << "------------" << endl;
    cout << "Toal tax " << totaltax << endl;
    cout << "Total price " << totalprice << endl;

    return 1;
}

While pasting the source code I have found it has become unreadable. Hence please download the source code from here.

Answer 1

class NonFoodBookMedicalItem : virtual public Item
{

I don't feel like this is the right approach, for several reasons. For a start, you've baked quite a bit of application logic right into the type name, which will be copied and reused throughout your code. How much work will be involved if the government decides that sales tax should apply to books? Every single usage of it will need to be updated to NonFoodMedicalItem.

As an aside: never assume that tax situations won't change. UK Value Added Tax (VAT) was 17.5% for as long as anyone could remember, and everyone's formulae had it hard-coded. Thousands of forms had the VAT rate element pre-printed with 17.5%. This worked fine until the government changed it in the wake of the global financial crisis.

Secondly, the naming is just awkward. Is it a non-food item that can be a book or medical item, or is it a non-food, non-book and non-medical item? The ordinary usage of the English language leaves this ambiguous.

class NormalItem: public ImportedItem, public NonFoodBookMedicalItem
{

The public inheritance from ImportedItem here is questionable. Public inheritance in C++ should be used to model the "is-A" relationship, i.e. if Foo publically inherits from Bar then it should be the case that every instance of Foo is a Bar. It's clearly not the case that every item is an imported item.

I'm not sure I see why you're using inheritance in the way you are, while at the same time using the strategy pattern. It looks like your aim was to employ the strategy pattern so that each instance delegated its tax calculations to its iSalesTax member object. The type of the object that the iSalesTax object points to can vary at run-time independently of the type of the object itself -- this is how the strategy pattern is supposed to work. But if you do this, I don't see the advantage in having a different static type for each tax situation. Should I be able to replace the iSalesTax in a book item with one that adds sales tax? If so, why declare a different type? If not, why use the strategy pattern?

Answer 2

I see a number of things that may help you improve your code. In no particular order, here's what I found:

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. It's especially bad when used within an included file.

Avoid switch statements within a class

The ItemCreator class has a big switch statement which does nothing but match arbitrary constants to arbitrary constructors. This is a strong signal that the class hierarchy is faulty. Either the different constructors should be entirely different classes or the arbitrary constants should be solely defined within the context that they're used.

Rethink the class hierarchy

The use of a strategy pattern is not inherently faulty here, but the execution could be better. Specifically, each Item has a corresponding SalesTax so what would make sense is to have a set of SalesTax objects, each using the relevant rates and calculations, and that individual Item objects would point to one of the four possible static SalesTax objects. If the tax code changes at some later date, all that will be required is to update references to SalesTax objects. (Alternatively, have a taxType attribute attached to each Item which could be used by a SalesTax object to apply the correct calculation.) If that's done, the Item class interface looks like this:

class Item
{
public:
    Item(double price, const SalesTax &aSalesTax) : iPrice(price), iSalesTax(aSalesTax) {}
    Item(Item &&a) : iPrice(a.iPrice), iSalesTax(a.iSalesTax) {}
    double getPrice() const { return iPrice; }
    double getTax() const { return iSalesTax(iPrice); }
    double getTotalPrice() const { return getPrice() + getTax(); }

protected:
    double iPrice;
    const SalesTax &iSalesTax;
};

The SalesTax class would look like this:

class SalesTax
{
public:
    SalesTax(double rate) : irate(rate) {};
    double operator()(const double price) const { return RoundOff(price * irate); }

private:
    double RoundOff(double aTax) const;
    double irate;
};

Use rational return values

It is odd that Item::CalculateTotalTax() return void. It makes more sense for it to return a double that represents the calculated tax. It also doesn't make sense to have a separate CalculateTotalTax and then a getTax function. What if the caller forgets to call them in that sequence? Better would be to have a single call which calculates and returns the tax. The same is true of the CalculateTotalPrice and getPrice functions.

Declare member functions as const

The getPrice() method doesn't (and shouldn't!) alter the underlying Item object, so it should be declared const to make it clear that it does not alter the object.

Use iterators fully

The code within main that calculates the final pricing and taxes, the code uses an iterator to traverse the Basket but then doesn't use it for anything else. Instead, it would be cleaner to use the actual iterator to make it clear what is being iterated and how it's being used. For example, the code could instead be written like this:

int pos = 1;
for (listOfItem::iterator it = Basket.begin(); it != Basket.end(); ++it, ++pos)
{
    totaltax += (*it).getTax();
    double price = (*it).getPrice();
    totalprice += (*it).getTotalPrice();
    std::cout << "Item " << pos << " price " << price << std::endl;
}

Use consistent naming

The convention you have mostly followed is one in which the class names begin with an uppercase letter, such as SalesTax and that class instances begin with a lowercase letter, such as st. However, you haven't used them consistently, so there is a Basket variable (which looks like a class name) and it is declared as a listOfItem type (which looks like a variable name).

Move long const strings to variables

Removing lengthy constant strings from within your program and assigning them to variables is an easy way to reduce clutter in your source code. For example:

const std::string itemPrompt(
    "Enter the type of Item...1,2,3,4\n" 
    "1 for ITEM_WITH_NOSALESTAX_AND_NOIMPORTDUTY\n"
    "2 for ITEM_WITH_NOSALESTAX_ONLY_IMPORTDUTY\n"
    "3 for ITEM_WITH_ONLY_SALESTAX_AND_NOIMPORTDUTY\n" 
    "4 for ITEM_WITH_BOTH_SALESTAX_AND_IMPORTDUTY\n" 
);
const std::string pricePrompt(
    "Enter the price of the Item\n"
);
const std::string continuePrompt(
    "Do you want to continue?... (y/n)\n"
);

Don't use std::endl unless you really need to flush the stream

The difference between std::endl and '\n' is that std::endl actually flushes the stream. This can be a costly operation in terms of processing time, so it's best to get in the habit of only using it when flushing the stream is actually required. It's not for this code.

Avoid using new and delete

With C++11 and C++14, the use of new and delete is much more rare than it once was. This is because we have things such as smart pointers and RAII, which should be used instead. Whole classes of errors can be eliminated this way. For example, consider this rewritten loop within main:

do
{
    std::cout << itemPrompt;
    unsigned type_of_item;
    std::cin >> type_of_item;
    std::cout << pricePrompt; 
    double price;
    std::cin >> price;
    if (--type_of_item < 4) {
            basket.emplace_back(price, tax[type_of_item]);
    }
    std::cout << continuePrompt;
    std::cin >> answer;
}
while(answer == 'y');

The various tax rates are created like this:

const SalesTax tax[4] = { SalesTax(0), SalesTax(0.05), SalesTax(0.1), SalesTax(0.15) };

Reserve non-zero return values for errors

The code currently ends with a return 1; but it would be much more typical to reserve non-zero values for error conditions and return 0; for the normal exit. In fact, this idiom is so common that you don't even need to write that line -- it will be generated automatically by the compiler.

Answer 3

I would use a diferent aproach instead of all this inheritance, I think it is simpler to use just Item class with Rate and ImportedDuty, on the factory just create and configure each item with the desired values.