6

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?

1
  • 4
    If it breaks the chain, it is not a real decorator...
    – Oded
    Commented Feb 29, 2012 at 20:45

3 Answers 3

3

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.

2
  • Thanks for a good, well-written answer. The reason behind implementing this cache object as a 'decorator' is because the templating system already uses the decorator pattern. Not strictly the decorator pattern? I agree, but I think I'm going to continue as is for now and if I run into any issues, start refactoring! Commented Mar 1, 2012 at 18:34
  • Where I work right now we use the decorator pattern frequently and a "caching decorator" is also very common; furthermore, each decorator extends ProxySomething (where Something is the interface being decorated) so that we don't need to redefine every method. I think the name "decorator pattern" is the problem. Call it that in your code so people recognize it, but also recognize that if the pattern is very useful for something that doesn't strictly seem like "decorating", don't let terminology alone deter you from doing it anyway. Things are named badly - trust experience and analysis. Commented Sep 8, 2022 at 21:27
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.

1

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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