Is this a valid Decorator pattern example

Question

Is this a valid example for decorator pattern?

I would like to know is this example a valid example for decorator pattern? if not what
is correction or change needed here, please suggest.

We have a Container class representing container and for it i want to add features at
run time. features like wheel and lid are used in the example.

Client code:

private void button2_Click(object sender, EventArgs e)
{
  IContainer container = new MovableConatiner(new Container());
  string features = container.getFeatures();
  container = new LidConatiner(container);
  features = container.getFeatures();
}

API code:

public interface IContainer
{
  string getFeatures();
}

public class Container : IContainer
{      
  public string getFeatures()
  {
    return "Container";
  }
}

// a decorator to add wheels
public class MovableConatiner : IContainer
{
  protected IContainer container;

  public MovableConatiner(IContainer container)
  {
    this.container = container;
  }

  public string getFeatures()
  {
    string features = container.getFeatures();

   features = features != string.Empty ? string.Format("{0} , four wheels", features) :       
   features;
   return features;
  }
}

// a decorator to add lid to contaner
public class LidConatiner : IContainer
{
  protected IContainer container;

  public LidConatiner(IContainer container)
  {
    this.container = container;
  }

  public string getFeatures()
  {
    string features = container.getFeatures();

    features = features != string.Empty ? string.Format("{0} , Lid", features) : 
    features;
    return features;
  }
}

its as per my understand so i would like to verify my understanding.

Answer 1

While your implementation doesn't ideally reflects the structure or the Decorator pattern, from my point of view it solves the same problem that Decorator is targeted to solve. Your solution is just not as strict and safe for future modifications as "ideal" Decorator implementation.

You simplified implementation removing "unnecessary" for you abstractions. But while you might think they are unnesessary at the moment, they will become very useful in the future when your application grows and get few dozens of components and decorators. it's easy to get lost who is who currently.

In the link I provided there is very simple description of main participants of the Decorator pattern, and there is a sample code, very similar to yourth, but complete. I won't copy-paste it here to show you corrent implementation.

I just want to stress that if you don't understand the need of some abstractions in Design patterns, you better leave them, or read once more how to use it instead of just removing them.

UPDATE: point in favour of abstractions:

All common logic must be implemented in one place and reused. Code duplication is very-very bad. I think everybody agrees that this is fundamental principle of well-organized code.

Now let's analyze your implementation of Decorator pattern without extracting abstract base class for decorators. Compare the implementation of your MovableContainer and LidContainer classes - do you see anything similar? I hardly see any difference at all actually. Ok, let's find what's common:

• both have constructors, receiving component to decorate.
• both store reference to IContainer which is decorated
• both retrieve component's features in the getFeatures() method
• both check component's features for empty and if not - append some string (the only difference is string itself)

All this logic should be extracted to base class. You already should udnerstand that base class for your two decorators is required.

Let's go further. Let's imagine every possible decorator for every possible container. As implies from the Decorator pattern definition, it's obvious that some logic is common for all decorators: they all store reference to the decorating object. Is that enough for extracting base class (single property - that's not hard to copy-paste it to both decorators you have)? Definitely enough!

If you like real-life examples, here it is. You've implemented few decorators, let's say 100 (2 is still enouth, 100 just exadurates the problem). And they you realize that some decelopers doesn't know how to use them properly - they just pass NULLs to your decorators. Their code works fine, then created decorators are passed somewhere else, or stored to DB etc. And then at some magical points, your code is failing in various places later. It's hard to every time find where that NULL came from, which part of application created such object. You decide to add NULL-check in the constructor to disallow passing NULLs and make initial code fail to immediately fix the problem. Ouch, we need to fix all 100 constructors! And merge your changes with changes of 10 more developers who are working everybody on his decorator. That's not a good perspective. If this example didn't convince you and you are still ready to copy-paste code 100 times, imagine that you are developing a reusable library and other developers from other companies also implement decorators derived from your IContainer. You have no way to fix constructors of their decorators and ensure they won't provide you with invalid object containint NULL internally. On contrary if you had a base class for Decorator, you just needed to fix it - and all implementations both yourth and 3rd party get that functionality. If you think you don't implement a reusable library - consider other developers working in your team as 3rd party - it's always useful and not so different - don't require them to change their code because you need some fix.

Finally I provide the way I would refactor your code (I didn't want to do it at the beginning to let you come to this on your own):

public interface IContainer
{
    string getFeatures();
}

public class Container : IContainer
{
    public string getFeatures()
    {
        return "Container";
    }
}

public abstract class ContainerDecorator : IContainer
{
    protected IContainer container;

    protected ContainerDecorator(IContainer container)
    {
        if (container == null)
        {
            throw new ArgumentNullException("container");
        }
        this.container = container;
    }

    public abstract string getFeatures();
}

public class StringFormatDecorator : ContainerDecorator
{
    private readonly string _format;

    public StringFormatDecorator(IContainer container, string format) : base(container)
    {
        _format = format;
    }

    public override string getFeatures()
    {
        string features = container.getFeatures();

        features = features != string.Empty ? string.Format(_format, features) : features;
        return features;
    }
}

// a decorator to add wheels
public class MovableConatiner : StringFormatDecorator
{
    public MovableConatiner(IContainer container) : base(container, "{0} , four wheels")
    {
    }
}

// a decorator to add lid to contaner
public class LidConatiner : StringFormatDecorator
{
    public LidConatiner(IContainer container) : base(container, "{0} , Lid")
    {
    }
}

Such code not only improves core reuse, but also prevents others from using your decorators in wrong way because of lost border between containers and decorators. It's much harder to declare parameterless decorator now and almost impossible to use it. You can't "decorate" one container with another container which is non-sense, but possible in your implementation when some new developer creates his own container without knowing your initial intentions. Now doing things wrong becomes much more complex.