Hypothetical SalesTax challenge

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

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.

StackExchange Code Review Q#531, answer score: 10