views:

60

answers:

6

I have the following scenario where I have different kinds of sales algorithms to calculate the sales price. FixedSaleStrategy does not need basePrice parameter while all the other strategy implementations need it. Is there a good way to avoid this redundant parameter?

public abstract class SalesStrategy
{
    public abstract double GetPrice(double basePrice, double saleAmount);
}
public class AmountOffSale : SalesStrategy
{
    public override double GetPrice(double basePrice, double salesAmount)
    {
        return basePrice - salesAmount;
    }
}
public class FixedPriceSale : SalesStrategy
{
    public override double GetPrice(double basePrice, double salesAmount)
    {
        return salesAmount;
    }
}
A: 

Not a good one, in my opinion. I would keep it as is. There are various tricks you could use like params (have a single params double[] priceData) or IDynamicObject. But the cleanest is just to have some strategies ignore the extra parameter.

Matthew Flaschen
+2  A: 

If you're using c# 4.0, you could reverse the parameters and make basePrice optional like so:

public abstract class SalesStrategy
{
    public abstract double GetPrice(double saleAmount, double basePrice = 0d);
}

public class AmountOffSale : SalesStrategy
{
    public override double GetPrice(double salesAmount, double basePrice)
    {
        return basePrice - salesAmount;
    }
}

public class FixedPriceSale : SalesStrategy
{
    public override double GetPrice(double salesAmount, double basePrice = 0d)
    {
        return salesAmount;
    }
}

Meaning the following can be done...

FixedPriceSale fixedPrice = new FixedPriceSale();
...
fixedPrice.GetPrice(salesAmount);

Note that AmountOffSale's basePrice parameter is not optional, meaning that the following will not compile:

AmountOffSale amountOffSale = new AmountOffSale();
...
// No overload for method 'GetPrice' takes 1 arguments
amountOffSale.GetPrice(salesAmount); 
robyaw
I just typed in this same response; then noticed your reply as I was verifying the code :-).
Jess
+5  A: 

No. It's not a redundant parameter; the code that utilizes a SalesStrategy should not know which concrete class it is using, so the method signature must be identical in all derived classes.

Jamie Ide
A: 

A good way to remove irrelevant parameters from an interface is by passing those parameters in constructors from subclasses . So, an alternative for your design would be:

public interface SalesStrategy
    {
        double CalculatePrice(double basePrice);
    }

public class FixedPriceSale : SalesStrategy
    {
        public double CalculatePrice(double basePrice)
        {
            return basePrice;
        }
    }

public class AmountOffSale : SalesStrategy
    {
        public double SalesAmount { get; set; }

        public AmountOffSale(double salesAmount)
        {
            this.SalesAmount = salesAmount;
        }

        public double CalculatePrice(double basePrice)
        {
            return basePrice - SalesAmount;
        }
    }

In that construction you do not pollute your interface with specific data from subclasses.

nandokakimoto
I think the problem with this approach is that at some point, something needs to create a `SalesStrategy` sub-class and **that** thing will need to know the basePrice. So effectively this moves the extra parameter from the method call to the factory method. There still would be a place where this parameter is not being used.
Igor Zevaka
+1  A: 

Another alternative is to use a parameters object or Dictionary<string, object>. This way you can consolidate the number of parameters on each method and leave room for additional parameters should there be a change in requirements in the future.

The one drawback is that a Dictionary<string, object> can make tracking the parameters harder in your code, where as a parameters object will simply have all the properties that you can view in your code.

David Robbins
+2  A: 

At the core of the strategy patten is the idea that the calling code does not know the implementation being called.

If you were to change the paramaters used per implementation you would find that you are not getting the benifit of this pattern: a caller would need to know which implementation was going to be used and how to call it.

What I tend to do is pass a class that contains a super-set of information (something like PricingInfo), which is always populated the same way (Ideally centerlized in the code) and the only difference is the implementaions of the strategy.

One of the benifits is that I can add a property to my PricingInfo class that was not relvant in the past (like say, systemDiscount), and the impact to the system as a whole is not too large.

brian chandley