7
\$\begingroup\$

I have a small solution to the following hypothetical 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.

When I purchase items I receive a receipt which lists the name of all the items and their price (including tax), finishing with the total cost of the items, and the total amounts of sales taxes paid. The rounding rules for sales tax are that for a tax rate of n%, a shelf price of p contains (np/100 rounded up to the nearest 0.05) amount of sales tax. Write an application that prints out the receipt details for these shopping baskets.

INPUT:

  • Input 1: 1 book at 12.49 1 music CD at 14.99 1 chocolate bar at 0.85
  • Input 2: 1 imported box of chocolates at 10.00 1 imported bottle of perfume at 47.50
  • Input 3: 1 imported bottle of perfume at 27.99 1 bottle of perfume at 18.99 1 packet of headache pills at 9.75 1 box of imported chocolates at 11.25

OUTPUT

  • Output 1: 1 book : 12.49 1 music CD: 16.49 1 chocolate bar: 0.85 Sales Taxes: 1.50 Total: 29.83
  • Output 2: 1 imported box of chocolates: 10.50 1 imported bottle of perfume: 54.65 Sales Taxes: 7.65 Total: 65.15
  • Output 3: 1 imported bottle of perfume: 32.19 1 bottle of perfume: 20.89 1 packet of headache pills: 9.75 1 imported box of chocolates: 11.85 Sales Taxes: 6.70 Total: 74.68

I'm interested in feedback on my use of design patterns, whether there are any extensibility issues, how SOLID the code is and the style/structure of the unit tests. I'd really appreciate hearing constructive criticisms.

Full solution is available here

This is part of the main solution:

public class SalesItem : ISalesItem
{
    private string _name;
    private decimal _price;

    public SalesItem(string name, decimal price)
    {
        #region Parameter Checking
        if (String.IsNullOrWhiteSpace(name))
            throw new ArgumentException("name");
        if(price < 0)
            throw new ArgumentException("price");
        #endregion

        this._name = name;
        this._price = price;
    }

    #region ISalesItem Members

    public string Name
    {
        get { return this._name; }
    }

    public decimal GetPrice()
    {
        return this._price;
    }

    public decimal GetSalesTax()
    {
        return 0.0M;
    }

    public decimal GetTotal()
    {
        return this.GetPrice() + this.GetSalesTax();
    }

    #endregion
}

public class SalesItemTaxDecorator: ISalesItem
{
    protected ISalesItem _decoratedSalesItem;
    protected ITax _salesTax;

    public SalesItemTaxDecorator(ISalesItem salesItem, ITax salesTax)
    {
        this._decoratedSalesItem = salesItem;
        this._salesTax = salesTax;
    }

    #region ISalesItem Members

    public string Name
    {
        get { return _decoratedSalesItem.Name; }
    }

    public virtual decimal GetSalesTax()
    {
        return this._decoratedSalesItem.GetSalesTax() + _salesTax.CalculateTax(this._decoratedSalesItem.GetPrice());
    }

    public virtual decimal GetPrice()
    {
        return this._decoratedSalesItem.GetPrice();
    }

    public virtual decimal GetTotal()
    {
        return this.GetPrice() + this.GetSalesTax();
    }

    #endregion
}

public class Tax : ITax
{
    private decimal _rate;
    private IRounding _rounding;

    public Tax(decimal rate, IRounding rounding)
    {
        this._rate = rate;
        this._rounding = rounding;
    }

    #region ISalesTax Members

    public decimal Rate
    {
        get { return this._rate; }
    }

    public IRounding Rounding
    {
        get { return this._rounding; }
    }

    public virtual decimal CalculateTax(decimal itemPrice)
    {
        decimal tempTax = itemPrice * this.Rate;
        decimal roundedTax = Rounding.Round(tempTax);
        return roundedTax;
    }

    #endregion
}

public static class SalesItemFactory
{
    private static readonly Rounding ROUNDING = new Rounding(0.05M);
    private static readonly ITax BASICTAX = new Tax(0.1M, ROUNDING);
    private static readonly ITax IMPORTTAX = new Tax(0.05M, ROUNDING);

    private static readonly Dictionary<ItemType, ITax> itemTaxLookup = new Dictionary<ItemType, ITax>()
    {
        { ItemType.Basic, BASICTAX },
        { ItemType.Import, IMPORTTAX }
    };

    public static ISalesItem GetSalesItem(string name, decimal price, ItemType itemType)
    {
        ISalesItem item = new SalesItem(name, price);

        foreach (int flag in Enum.GetValues(typeof(ItemType)))
        {
            if ((flag & (int)itemType) == flag)
            {
                item = (ISalesItem)Activator.CreateInstance(typeof(SalesItemTaxDecorator), new object[] { item, itemTaxLookup[(ItemType)flag] });
            }
        }

        return item;
    }

    public static ISalesItem GetSalesItem(string name, decimal price)
    {
        return new SalesItem(name, price);
    }
}
\$\endgroup\$
  • \$\begingroup\$ This precise homework problem has been addressed before in the StackExchange universe. \$\endgroup\$ – radarbob Mar 13 '16 at 23:49
10
\$\begingroup\$

Wow, a lot of code to review. Unfortunately I'm too lazy to look through entire solution, let's I'll post my first 10 points.

1) http://en.wikipedia.org/wiki/Overengineering. Looked through the solution and have found absolutely no point in having such an amount of interfaces. I would definitely get rid of ITax and IRounding because there is only one implementation for each of them and there is no point to isolate them during unit-testing because there is too small functionality to isolate. Do not make it in such a complicated way until you really need such a complicated model. You will have time to make things worse later :)

2) I personally hate 4-line regions - they do not help at all as for me.

3) If you have a factory for salesItems then why don't you want to make class SalesItem hidden inside Factory class.

public static class SalesItemFactory
{
    class SalesItem : ISalesItem { ... }
    class SalesItemTaxDecorator : ISalesItem { ... }

    ...
}

If you really want to unit-test them then at least make them internal and use InternalsVisibleTo. Or better even not to test them, test factory!

4) item = (ISalesItem)Activator.CreateInstance(typeof(SalesItemTaxDecorator), new object[] { item, itemTaxLookup[(ItemType)flag] });

Why activator at all?

5) The same line as in previous point. Why do you assign this value to item and continue iterations? Simply return it! And in order to keep your logic you should revert collection returned by Enum.GetValues.

6) I would remove 'protected' and 'virtual' stuff from your decorator and replace them with 'private' and 'sealed' instead. Otherwise you're giving an ability to decorate decorator but that is strange. Too much of extensibility

7) Let's go to unit-tests :). https://github.com/manwood/SalesTax/tree/master/Manwood.SalesTax.Tests/Stubs. Do not write your stubs, use for example RhinoMock instead. They make life easier due to many reasons: you do not have to write your stubs, you will not see your stubs while looking for inheritors of ITax, they allow you to write more precise unit-tests (your unit-tests are not that good, see next point).

8) https://github.com/manwood/SalesTax/blob/master/Manwood.SalesTax.Tests/SalesItemTest.cs.

[TestClass]
public class SalesItemTest
{
    [TestMethod] public void SalesItemGetPriceTest() { }
    [TestMethod] public void SalesItemGetSalesTaxTest() { }
    [TestMethod] public void SalesItemGetTotalTest() { }
}

Doubling your names too much in testMethods. PriceTest(), SalesTaxTest(), GetTotalTest() would be better names.

9) https://github.com/manwood/SalesTax/blob/master/Manwood.SalesTax.Tests/SalesItemTaxDecoratorTest.cs

    [TestMethod]
    public void SalesItemTaxDecoratorTotalTest()
    {
        SalesItemTaxDecorator taxDecorator = new SalesItemTaxDecorator(new SalesItemStub(10M, 0M), new TaxStub(0.1M));
        decimal total = taxDecorator.GetTotal();
        Assert.AreEqual(11M, total);
    }

This is invalid test. What you're testing is whether it will return 11 if price is 10 and tax is 0.1. What you should test is a) Tax was calculated for price value (it means that ITax.CalculateTax was invoked with parameter 10) b) result taken from tax was added to 10 and returned. RhinoMocks will help use assert a) statement and you should assert b) on your own in the same manner you do it right now. This test should not have 0.1 constants, otherwise you're testing tax class also (even though it is a stub).

10) 10th point, I want to go sleep.

Asking about patterns in such a simple example will never be easy, because there is no point in them in such examples and you will never need them until you will extend it.

\$\endgroup\$
  • \$\begingroup\$ Hey, thanks for your thoughts. The point about over-engineering and use of patterns is clearly applicable to such a small example, but using them here was more about applying them correctly so that the example could be extended with minimal modification. If you wanted new tax types for example, the system would support the Open/Closed principle. \$\endgroup\$ – manwood Feb 1 '11 at 23:30
  • \$\begingroup\$ Point 5 - because the enum ItemType is comprised of flags and the item can be any number of types, so the nesting of item allows me to wrap the original item in 'decorators'. Point 7/9 does using RhinoMocks mean you do not need stubs at all? I take you point about SalesItemTaxDecoratorTotalTest() - that was one I was really uncomfortable with and can see how a mocking framework would help here. Lastly, on the subject of patterns, how would you have solved the problem of items being multiple types? (eg, Basic, Exempt, Import... SomeOtherTypeYouCreateInTheFuture etc)? \$\endgroup\$ – manwood Feb 1 '11 at 23:33
  • \$\begingroup\$ (Also I don't see how we can demonstrate design patterns in a brief codereview site like this without using small, hypothetical examples in which their use does constitute overengineering!) \$\endgroup\$ – manwood Feb 1 '11 at 23:36
  • \$\begingroup\$ @manwood, striked out point 5, didn't mention that you're wrapping item over and over. on how to review patterns' usage - I'not sure, maybe less code more class/interface definitions? \$\endgroup\$ – Snowbear Feb 1 '11 at 23:43
  • 1
    \$\begingroup\$ Just wanted to notice that I only partially agree with point 2 because the regions that are used are automatically generated by visual studio upon implementing an interface. Which is a-ok in my book. \$\endgroup\$ – Peter Feb 4 '11 at 15:56

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.