Problem
Is this a good usage of Decorator pattern? I’m trying to use Interfaces instead of Abstract classes.
(Component) Interface
public interface IBake
{
string Name { get; }
double Price { get; }
}
(Concrete Component)
public class BakeBase : IBake
{
private string m_Name = "Cake Base";
private double m_Price = 200.00;
public string Name
{
get
{
return m_Name;
}
}
public double Price
{
get
{
return m_Price;
}
}
public string GetCakeCost()
{
return "Normal cake is:" + m_Price + "and Tax is: " + (this.m_Price * 0.08f);
}
}
Decorator
class CherryDecorator
{
IBake _iBakeModel;
public CherryDecorator(IBake iBakeModel)
{
_iBakeModel = iBakeModel;
}
public string GetName()
{
return "Cherry Decorated Cake!";
}
public double GetPrice()
{
return _iBakeModel.Price + 100;
}
}
Decorator
class CreamDecorator
{
IBake _iBakeModel;
public CreamDecorator(IBake iBakeModel)
{
_iBakeModel = iBakeModel;
}
public string GetName()
{
return "Cream Decorated Cake!";
}
public double GetPrice()
{
return _iBakeModel.Price + 50;
}
}
Client
public class Client
{
static void Main(string[] args)
{
BakeBase bakebase = new BakeBase();
Console.WriteLine(bakebase.GetCakeCost());
Console.ReadKey();
Console.WriteLine(Environment.NewLine);
CherryDecorator cd = new CherryDecorator(bakebase);
Console.WriteLine(cd.GetName() + " " + cd.GetPrice());
Console.ReadKey();
Console.WriteLine(Environment.NewLine);
CreamDecorator crd = new CreamDecorator(bakebase);
Console.WriteLine(crd.GetName() + " " + crd.GetPrice());
Console.ReadKey();
}
}
I’m getting the following, which is not the right total:
Solution
You missed a somewhat crucial point in the pattern: the decorator must present the exact same interface as the decorated object.
In other words, any IBake
decorator should also implement the IBake
interface, so that it’s possible to decorate a cake with both cream and cherries!
Here’s your CreamDecorator
, fixed:
class CreamDecorator : IBake
{
public static readonly decimal ExtraCharge = 50;
private readonly IBake _decorated;
public CreamDecorator(IBake decorated)
{
_decorated = decorated;
}
public string Name
{
get
{
var name = _decorated.Name;
return name + " +Cream";
}
}
public double Price
{
get
{
var price = _decorated.Price;
return price + ExtraCharge;
}
}
}
The idea is that you call the decorated code, and then decorate the result before returning it.
Also since we’re talking about money, double
is a bad choice of a type for Price
. IBake
should look like this:
public interface IBake
{
string Name { get; }
decimal Price { get; }
}
As for the wrong price being output, there’s your bug:
public string GetCakeCost()
{
return "Normal cake is:" + m_Price + "and Tax is: " + (this.m_Price * 0.08f);
}
The base class doesn’t know whether it’s decorated or not, and shouldn’t make that assumption, it’s just another implementation of the IBake
interface:
public static readonly decimal TaxRate = 0.08;
public string GetCakeCost()
{
return string.Format("Price: {0:C2}nTax: {1:C2}nTotal: {2:C2}", Price, Price * TaxRate, Price * (1 + TaxRate));
}
Now that looks cleaner, but it’s also flawed: Cost
is a bad/confusing term, should be Price
, SellingPrice
, or TotalPrice
– Cost
could refer to what the bakery is paying to produce the cake, and would be used to calculate profits and gross margins. The tax rate shouldn’t be hard-coded into a method that returns a string
either.
Another problem is that the decorator would need to override the Price
property for the base class to return the correct figure, and that’s not how decorators work.
One solution could be to include a TaxAmount
member in the IBake
interface, and a separate member to return a string
representation of the cake’s price, which I’d call PriceDetails
:
public interface IBake
{
string Name { get; }
decimal Price { get; }
decimal TaxAmount { get; }
string PriceDetails { get; }
}
The base cake could do it like this:
public decimal TaxAmount
{
get { return TaxRate * Price; }
}
public string PriceDetails
{
get { return return string.Format("{0}nPrice: {1:C2}nTax: {2:C2}nTotal: {3:C2}", Name, Price, TaxAmount, Price + TaxAmount); }
}
And then the decorators have nothing to change about these two, so they just call the decorated code:
public static readonly decimal ExtraCharge = 50;
public decimal TaxAmount
{
get { return _decorated.TaxAmount; }
}
public string PriceDetails
{
get { return _decorated.PriceDetails; } // add an "Extra" in the detail line?
}
At the end of the day, what you want to ensure is that the client code is always calling into the decorated instance – that’s why it’s important for decorators to present the same interface as the decorated type, and for all members to be accessed by the client code via that interface.
Here’s what your client code could look like in a production application:
public class CakeOrderLine
{
private readonly IBake _cake;
public CakeOrderLine(IBake cake)
{
_cake = cake;
}
public override string ToString()
{
return _cake.PriceDetails;
}
}
An “order line” is injected with an IBake
instance, but has no clue how/if it’s decorated, it treats all implementations of IBake
equally.
Your console client could do this:
var cake = new BakeBase();
var cherryCake = new CherryDecorator(cake);
var creamCherryCake = new CreamDecorator(cherryCake);
And then this:
Console.Write("{0:C2}n", cake.PriceDetails);
Console.Write("{0:C2}n", cherryCake.PriceDetails);
Console.Write("{0:C2}n", creamCherryCake.PriceDetails);
Should output something like this, assuming Price
is implemented as above:
Cake Base
Price: $200.00
Tax: $16.00
Total: $216.00
Cake Base +Cherry
Price: $250.00
Tax: $20.00
Total: $270.00
Cake Base +Cherry +Cream
Price: $350.00
Tax: $28.00
Total: $378.00
I’m not a UML-ist, I think this fairly represents the concept:
See the full code here.
As I’m a more java programmer than C# but I try to answer you as good as I can.
The decorater pattern should be able to decorate multiple times.
Example : I must be able to decorate with cream and cherry and cream again.
In your implementation you can only decorate once.
Your decoration class have to implement the IBake
interface to so that your test should be :
IBake bakebase = new BakeBase();
Console.WriteLine(bakebase.GetCakeCost());
Console.ReadKey();
Console.WriteLine(Environment.NewLine);
IBake cd = new CherryDecorator(bakebase);
Console.WriteLine(cd.GetName() + " " + cd.GetPrice());
Console.ReadKey();
Console.WriteLine(Environment.NewLine);
IBake crd = new CreamDecorator(bakebase);
Console.WriteLine(crd.GetName() + " " + crd.GetPrice());
Console.ReadKey();
Console.WriteLine(Environment.NewLine);
IBake creamWithCherry = new CherryDecorator(crd);
Console.WriteLine(crd.GetName() + " " + crd.GetPrice());
Console.ReadKey();
First, I’d name your Interface Pastry since it’s the abstraction of what you’re using. Second, let’s refer to the design “blueprint” of the pattern in UML. You’ve missed a few things:
- PastryDecorator is an abstraction of the decorators. This is useful but not necessary to avoid repeating code in each decoration.
- Decorators decorate something – in this case, a Pastry – but you don’t show the aggregation in your code.
- The
getCost()
method of the decorator first has to call the thing it’s decorating, and then “decorate” the answer (add the cost) before returning its own answer. Again in UML: