I know the title is weird, so let me try to do some basic setup.
I have an object called StyleBundle. Based on two things, the Duration of the StyleBundle, and the "type" of the StyleBundle (Unlimited or PerStyle), will determine the overall Price of the StyleBundle. So, here is a quick snipped of StyleBundle:
public class StyleBundle
{
public decimal Price {get; set;}
public StyleType Type {get; set;} //STyleType is a simple enum, either "Unlimited" or "PerStyle"
public Duration Duration {get; set;}
}
Here is Duration. Basically, it has an enum which is DurationType, which can be values like DurationType.OneYear, DurationType.TwoYears, etc...
public class Duration
{
public Duration(TimeSpan timeSpan, string name, DurationType type)
{
this.TimeSpan = timeSpan;
this.Name = name;
this.Type = type;
}
public TimeSpan TimeSpan { get; set; }
public string Name { get; set; }
public DurationType Type { get; set; }
}
In my StyleBundle class, I pass Duration to a Strategy factory called StyleBundlePricingStrategy. Here is that class:
public class StyleBundlePricingFactory
{
public static IPricingStrategy GetPricing(Duration duration)
{
if (duration.Type == DurationType.OneYear) { return new OneYearPricingStrategy(); }
if (duration.Type == DurationType.TwoYear) { return new TwoYearPricingStrategy(); }
etc...
etc...
}
}
the classes being returned implement the IPricingStrategy interface:
public interface IPricingStrategy
{
decimal GetPriceFor(StyleBundle aStyleBundle);
}
Which gets the Price for a StyleBundle. Each strategy class encapsulates how a price is retrieved for a given DurationType. Here is an example of the OneYearPricingStrategy class:
public class OneYearPricingStrategy : IPricingStrategy
{
public decimal GetPriceFor(StyleBundle aStyleBundle)
{
if (aStyleBundle.StylePricingType == StylePricingType.PerStyle)
{
return aStyleBundle.Products.Count() * 2500m;
}
else
{
return 50000m;
}
}
}
Okay, so pretty basic Strategy setup.
The thing that's eating me up is that if you look at this line of code in the "OneYearPricingStrategy" class:
if (aStyleBundle.StylePricingType == StylePricingType.PerStyle)
you'll see that I still have to use a conditional in the Strategy class to account for the StyleBundle type. The type of StyleBundle will affect how the Price is calculated.
To me, this is bad design, b/c for each strategy class I write "OneYearPricingStratety", "TwoYearPricingStrategy", etc... the StylePricingType conditional gets copied and pasted to all of the Strategy classes.
This is not good, b/c what happens when I have to add a new StylePricingType? I'll have to go back into each PricingStrategy class and update the code, so there goes the whole SRP out the window (along with other things)...
what I need is a way to implement some type of pattern that will allow me to combine the two "stratagies" (the Duration with the StyleBundleType) and allow the rules to live in one place... not to be strewn across code.
It's easy to digest the STrategy pattern when you're implementing for one Strategy, but this is a combination of two, and i know the way I have it written now is not a good practice, and does not accomplish what I want it to.
Maybe it's the wrong pattern?
Any pointers would be more than appreciated.
Thanks, Mike
EDIT:
in reaction to Garret's answer, I want to provide some more detail on how I got to the Strategy pattern in the first place. I did not shoehorn the implementation into Strategy first, but saw what I thought was a code smell, and decided the Strategy pattern could help me.
Initially, the StyleBundle.Price property used to look like this:
public decimal Price
{
get
{
if (this.StylePricingType == StylePricingType.PerStyle)
{
if (this.Duration.Type == DurationType.ThreeDays)
{
_price = 1500m;
}
else if (this.Duration.Type == DurationType.OneYear)
{
_price = 2500m;
}
else if (this.Duration.Type == DurationType.TwoYears)
{
_price = 2000m;
}
else if (this.Duration.Type == DurationType.ThreeYears)
{
_price = 1650m;
}
}
else if (this.StylePricingType == StylePricingType.Unlimited)
{
if (this.Duration.Type == DurationType.ThreeDays)
{
throw new Exception("You can not have a StyleBundle of type Unlimited for a Duration of three days.");
}
else if (this.Duration.Type == DurationType.OneYear)
{
_price = 50000m;
}
else if (this.Duration.Type == DurationType.TwoYears)
{
_price = 40000m;
}
else if (this.Duration.Type == DurationType.ThreeYears)
{
_price = 33500m;
}
}
else
{
throw new Exception("Illegal StylePricingType passed to Product.");
}
return _price;
}
private set
{
_price = value;
}
}
I saw that any time I would add another Duration type, I would need to come into StyleBundle and change code... to me, that seemed like motivating principle enough to seek a better solution.
Now, with the application of the Strategy design pattern to this problem, my StyleBundle.Price property looks like this:
public decimal Price
{
get
{
return _pricingStrategy.GetPriceFor(this);
}
private set
{
_price = value;
}
}
where _pricingStrategy is IPricingStrategy, and the decision of which implementor to new up is decided on by calling the StyleBundlePricingFactory.GetPricing(duration) class in StyleBundle's constructor.
Don't try to think of a pattern before you write code. Code then refactor. Occasionally you will refactor into a pattern.
The Strategy pattern is typically reserved for when you want to delegate the behavioral logic for a class. For instance, if I have a class ChessPlayer, then Grandmaster implements ChessStrategy and Novice implements ChessStrategy would be a good way to change the behavioral strategy of my ChessPlayer, while the interface of ChessPlayer that moves pieces around would not have to change.
In your case, you simply have data, not a complex price-calculating strategy, so I would look for an appropriate data structure. A doubly nested HashMap over Durations X PricingStyles would work fine. Example:
Price calculatePrice(Duration d, PricingStyle s) {
return map.get(d).get(s);
}
PS: When it comes to good design, whatever takes less code is usually the winner.