Help with strategy pattern

Question

I've been going through Head First Design Patterns (just came in recently) and I was reading about the strategy pattern, and it occurred to me that it might be a great way to implement a common way of calculating taxes etc. on all of the particular objects I use at work, but I had a question about it.

Here's what I was thinking:

public interface ITax
{
    decimal ProvincialTaxRate { get; set; } // Yes, I'm Canadian :)
    decimal CalculateTax(decimal subtotal);
}

public SaskatchewanTax
{
    public decimal ProvincialTaxRate { get; set; }

    public SaskatchewanTax()
    {
        ProvincialTaxRate = new decimal(0.05f);
    }

    public decimal CalculateTax(subtotal)
    {
        return ProvincialTaxRate * subtotal + FederalTaxRate * subtotal;
    }
}

public OntarioTax
{
    public decimal ProvincialTaxRate { get; set; }

    public OntarioTax()
    {
        ProvincialTaxRate = new decimal(0.08f);
    }

    public decimal CalculateTax(decimal subtotal)
    {
        return ProvincialTaxRate * subtotal + FederalTaxRate * subtotal;
    }
}

You may have noticed that there is no declaration of FederalTaxRate and that's what I wanted to ask. Where should that go?

• Passing it in to the constructor for each concrete ITax seems redundant and would allow for incorrect behaviour (all tax calculators must share the exact same federal tax rate).
• Likewise, creating a member of ITax would allow them to be inconsistent as well.

Should all tax calculators inherit from some other class where it's defined statically as well as ITax?

public class TaxCalculator
{
    public static decimal FederalTaxRate = new decimal(0.05f);
}

Answer 1

In my opinion, you have the right solution - create a base class that contains the Canadian federal fax rate from which all of your derived classes can inherit. Statically defining it is a perfectly fine idea. You could also make the FederalTaxRate define only an accessor function for the tax rate, so that you could presumably define it at runtime from a file or something else.

I don't think that this is uniquely the best solution, but it will work perfectly well. Design patterns shouldn't get in the way of your common sense, and I think that common sense will solve this problem just fine.

Answer 2

A few points:

1. ProvincialTaxRate should almost certainly be immutable at the interface level (no set property). Changing tax rates around doesn't seem like a good idea, although this means you can't use the auto-properties in your implementation.

2. If there's only one FederalTaxRate and it's just a simple numeric value, I think the abstract base class is a safe approach.

3. Less advisable: Depending on how taxes work, you could argue that CalculateTax depends on FederalTaxRate and therefore this needs to be supplied as a parameter (perhaps there are different FederalTaxRates and you don't want CalculateTax to have to know about them).

Don't let the definition of a design pattern get in the way of a good idea. They're patterns, not formulas. ;)

---

P.S. I'm an American, so if Canadian taxes are actually that simple, I hope the IRS takes a page from your book next year!

Answer 3

You might want to start with this code, and move on from there:

public interface ITax
{
    decimal CalculateTax(decimal subtotal);
}

public class SaskatchewanTax : ITax
{
    private readonly decimal provincialTaxRate;
    private readonly decimal federalTaxRate;

    public SaskatchewanTax(decimal federalTaxRate)
    {
        provincialTaxRate = 0.05m;
        this.federalTaxRate = federalTaxRate;
    }

    public decimal CalculateTax(decimal subtotal)
    {
        return provincialTaxRate * subtotal + federalTaxRate * subtotal;
    }
}

public class OntarioTax : ITax
{
    private readonly decimal provincialTaxRate;
    private readonly decimal federalTaxRate;

    public OntarioTax(decimal federalTaxRate)
    {
        provincialTaxRate = 0.08m;
        this.federalTaxRate = federalTaxRate;
    }

    public decimal CalculateTax(decimal subtotal)
    {
        return provincialTaxRate * subtotal + federalTaxRate * subtotal;
    }
}

At this point there may not be much point to have two different strategy objects representing the tax calculation, but with a more realistic implementation (I am assuming tax calculation is more complicated and varies more by province), it might make sense.

However, you should consider applying the "simplest thing that could possibly work" principle, and only use the strategy pattern when you feel that it is needed.

Answer 4

I think this is a common case of pattern abuse.

If you check your two "strategies", they do EXACTLY the same thing. The only thing that changes is the ProvincialTaxRate.

I'd keep things DRY and don't overuse this pattern (or any other), here you gain a little bit of flexibility, but then you also have 2 classes that don't pull their weights, and probably You Ain't Gonna Need that flexibility.

This is common when you learn a new technology or insight, you want to apply it everywhere (it happens to every one of us), even if doing it harms the code readability and maintainability.

My opinion: keep it simple

Regards

EDIT (In response to the author comment on my answer)

I did not try to make fun of you, or anyone. This is a common mistake, I did it MANY times, and learned it the hard way, not only with patterns but also with fancy frameworks, servers, new buzzword technologies, you name it.

The authors of the book themselves warn the readers not to overuse patterns, and the upvotes in this answer clearly indicate something too.

But if for some reason you still want to implement the pattern, here's my humble opinion:

• Make a superclass for both strategies, this superclass would be abstract and should contain the shared rate value of their child strategies (FederalTaxRate)

• Inherit and implement the abstract method "Calculate" in each subclass (here you'll see that both methods are the same, but let's continue)

• Try to make each concrete strategy immutable, always favor immutability as Joshua Bloch says. For that, remove the setter of ProvincialTaxRate and specify the value on it's constructor or directly in its declaration.

• Lastly, I'd create some static factory methods in the StrategySuperclass so that you decouple your clients from the implementations or concrete strategies (that can very well be protected classes now)

Edit II: Here's a pastie with some (pseudo) code to make the solution a bit more clear

http://pastie.org/441068

Hope it helps

Regards

Answer 5

Why don't you forget about Interfaces, and just use inheritance for what you can:

public abstract class Tax
{
    protected decimal ProvincialTaxRate; // Yes, you are Canadian ;)
    public decimal CalculateTax(decimal subtotal)
    {
        return ProvincialTaxRate * subtotal + FederalTaxRate * subtotal;
    }
    decimal FederalTaxRate = new decimal(0.20f);
}

public class SaskatchewanTax : Tax
{
    public SaskatchewanTax()
    {
        base.ProvincialTaxRate = new decimal(0.05f);
    }

}

public class OntarioTax : Tax
{
    public OntarioTax()
    {
        base.ProvincialTaxRate = new decimal(0.08f);
    }

}

If you need the interface, just implement it in the base class, and just use the derived classes for custom behaviour/behavior.

Answer 6

Just food for thought - what's wrong with putting this method in an appropriate class, and just calling it?

public decimal CalculateTax(decimal subtotal, decimal provincialTaxRate, decimal federalTaxRate) {
    return provincialTaxRate * subtotal + federalTaxRate * subtotal;
    }

I understand that you will want to use a different provincial rate for each province, but surely that shouldn't be hard-coded in an interface implementation?