Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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:

enter image description here

share|improve this question
    
Why is your indentation inconsistent amongst each code block? –  Jamal Jun 4 at 14:13
    
I've rolled back your edit as it appeared to invalidate answers. Please don't update the original code based on answers. –  Jamal Jun 4 at 14:46

3 Answers 3

up vote 16 down vote accepted

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:

UML for the below linked code

See the full code here.

share|improve this answer
    
Hmm....First decorator runs but....I'm getting a StackOverflowException on Second Decorator... –  TheTruthIsInCode Jun 4 at 16:32
    
Can you add the client code to your answer? –  TheTruthIsInCode Jun 4 at 17:36
    
@TheTruthIsInCode added some client code, with what I'd expect the output to be (slightly modified, I added Name in the PriceDetails). –  Mat's Mug Jun 4 at 18:02
    
This prints out the Base Cake Price 3 times. –  TheTruthIsInCode Jun 4 at 18:19
    
Let us continue this discussion in chat. –  Mat's Mug Jun 4 at 18:20

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();
share|improve this answer
    
Getting Casting error from CherryDecorator to IBake.. –  TheTruthIsInCode Jun 4 at 14:26
3  
that's why I put that your decoration classes has to implement the IBake interface –  chillworld Jun 4 at 14:46

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:

Decorator UML class diagram

  • 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:

nested decorated calls in UML sequence diagram

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.