Is it bad practice for a decorator to break the decoration chain?

Question

I'm currently using decorators to apply various modifications to a template prior to rendering it.

I'm unsure about my cache decorator, though, because it is currently able to break the chain of decorators and return a cached object as opposed to the original decorated object.

class Cache : Decorator
{
    public String Render()
    {
        if(templateCache.IsCached('some_template_identifier'))
        {
            return templateCache.Get('some_template_identifier');
        }
        String renderedTemplate = Template.Render();
        templateCache.Cache(renderedTemplate);
        return renderedTemplate;
    }
}

Is this bad practice? Should I always be returning the original decorated object or is it ok to break the flow like this and return a different object?

Answer 1

This doesn't seem to be breaking any chain to me.

The intent of the Decorator is that your calling code should be able to render your string just fine, if it's holding the Template object, or cached if it's holding the Cached object. It shouldn't know or care how many objects are wrapped around the basic object. The calling code should simply call Render and handle what it gets.

Breaking the chain would be if your Template's Render method had a different signature from the Cached Render method, so that the calling code needs to know which it has.

All this said, I also don't feel this is, by strict definition, a Decorator pattern.

Decorators tend to decorate whatever they're rendering, rather than change where it's getting that string from. So you might have had a QuotesWrapper and an LCaseWrapper so that if your Template returns Hello World!, you might wrap it in QuotesWrapper to get "Hello World!" or the LCaseWrapper to get hello world! or in both to get "hello world!". All the time, the calling code doesn't care.

What you're doing is similar but not the same. In no way are you decorating anything. If anything, it's more like a Proxy, at least in AOP terms.

Answer 2

No, I don't believe that a decorator should ever break the chain. Essentially, if your Decorator doesn't decorate, then it isn't a Decorator. *8')

As it is, I'm not sure what behaviour you were expecting a cached decorator to exhibit. I would suggest that a Cached Decorator should cache the decorations to an object, and not a decorated object itself.

That way, when you wrap an object in your cached decorator, it decorates the object with the same set of decorations that your template object has/had.

Answer 3

I assume, what you meant when you said "Breaking the decorator chain" = "You are discarding the object which is passed to "class Cache" and returning your own

Which is technically incorrect to do and should be avoided, because

1) Decorator is a structural pattern, which changes the structure dynamically. Caching is not the same purpose as a decorator, do you see the fundamental issue here?

2) Decorator adds/modify something to the seed object passed to it before returning it, by doing cache you are not adding any thing to it

How to solve this then? options are 1 - Make sure the object is taken from cache before calling the decorator chain and enforce it by putting it in you data retrieval method

2 - Have one more level of inheritance (though i do not recommend) before all these decorators, in which you all ways try to get the object from the cache, before calling a virtual method of the derived decorator's method to Render()

that is my 2 cents

~Basav