Decoration Freak, or OCP in action?

I'm writing C# code to read and eventually parse VB6/VBA code modules. I have an interface like this:

/// 
/// An abstraction that represents an object 
/// that can read a code file and return its content.
/// 
public interface ICodeFileReader
{
    /// 
    /// Reads and returns the contents of the specified code file.
    /// 
    /// The code file to read.
    /// Returns all lines in the code file.
    string[] ReadFile(string path);
}

Implemented like this:

/// 
/// An object that can read a code file and return its content.
/// 
public class CodeFileReader : ICodeFileReader
{
    /// 
    /// Reads and returns the contents of the specified code file.
    /// 
    /// The code file to read.
    /// Returns all lines in the code file.
    public string[] ReadFile(string path)
    {
        return File.ReadAllLines(path);
    }
}

That's all nice and works super well, I can write code that has a dependency on an ICodeFileReader, and write unit tests for that code that pass in a Mock, and the tests won't hit the file system and the path becomes irrelevant.

The CodeFileReader class is rock-solid IMO, it does one thing and does it well... but then what if there are other concerns to address, like validating that the file has a specific header content for example, or whatever else could come to mind - e.g. catching and logging any exception that could be thrown in the process.

Because the requirements aren't exactly crystal-clear at this point in the project, and I want to follow SOLID principles and write code that is open for extensibility, but closed for modification, I wrote a decorator base class:

```
public abstract class CodeFileReaderDecorator : ICodeFileReader
{
private readonly ICodeFileReader _reader;
protected ICodeFileReader Reader { get { return _reader; } }

protected CodeFileReaderDecorator(ICodeFileReader reader)
{
_reader = reader;
}

public virtual string[] ReadFile(string path)
{

The Decorator Pattern - Part 1: DRY (ExceptionLoggerCodeFileReader)

At fist glance, the decorator pattern is nice in that it gives the appearance of very easy adherence to the open/closed principle (OCP) and the single responsibility principle (SRP), but it does have its limitations.

The most obvious one is that any decorator is inevitably tightly coupled to its interface. If you find yourself writing code in a decorator that actually doesn't really have anything to do with the specific interface it's implementing, that should set off alarm bells because you're inevitably setting yourself up for a future violation of another important programming principle- Don't Repeat Yourself (DRY).

Your ExceptionLoggerCodeFileReader is a great example of this. The name is the first sign of this- instead of being one thing, that's two totally unrelated things stuck together, an ExceptionLogger and a CodeFileReader. Does exception logging have anything to do with reading code files? And if not then why is an exception logger being forced to implement the ICodeFileReader interface?

Nothing in that class is specific to the interface it implements apart from the method that is wrapped in a try/catch. So the next time you write a Foo class, and want an ExceptionLoggerFoo decorator for that class, you will find that almost all of the code in ExceptionLoggerCodeFileReader will need to be duplicated, hence the DRY violation.

The Decorator Pattern - Part 2: Separate and Dynamic Responsibilities (FileHeaderValidatorCodeFileReader)

In your answer, you quote a nice list of situations in which the decorator pattern is appropriate. What's interesting is what is and isn't on this list, compared to why you've employed it in this situation.

On the list:

• Add responsibilities dynamically

• Add responsibilities which can be withdrawn

Not on the list:

• Write code which adheres to the open/close principle.

So, let's take FileHeaderValidatorCodeFileReader as an example here. Is this a responsibility that only sometimes applies to reading code files? Is it a function that you want to add or withdraw based on some condition that can't be known at compile-time? I don't know the full workings of your program, but from how you've described it, I understand the answer to that to be no.

So given that, you now actually have a single responsibility (reading a code file, which must involve validating the header) split into three places: CodeFileReader, FileHeaderValidatorCodeFileReader and wherever you compose your objects together, which in the case of an IoC container is probably all the way up at the application route. The logic for when the file header validation is applied (which in this case, as far as I can tell, is simply that it is always applied) has to live far away from the class which should actually be responsible for it, instead sitting up in the object graph composition route.

The Alternatives - Part 1: Aspect-Oriented Programming (ExceptionLoggerCodeFileReader)

So what about OCP? Because despite any other problems, it's still the case that the decorator pattern does give very good adherence to that pattern. Can that be maintained without using decorators?

First, to deal with the ExceptionLoggerCodeFileReader. This kind of logging is generally difficult to do in a way that adheres to good design principles. A decorator allows it- and any dependencies it brings like your ILogger- to at least be separated out from the rest of the body of the class, but with or without a decorator, you will inevitably find yourself repeating a lot of code in repeatedly writing the try...catch{log,rethrow} pattern.

The reason for this is that exception logging is a cross-cutting concern- a concern which is involved with many other concerns in a way which is difficult to separate out. And this is precisely the key use case for Aspect-oriented programming.

When working with certain frameworks like ASP.NET's MVC, there's some built in support for aspect-oriented programming (in that case, using action filter attributes). But the general way to apply it is through an IoC container which supports interceptors. I won't go too much into the implementation details because that will vary from container to container, but to give an overview, here's an example using Ninject's Interception extension:

public abstract class BaseInterceptor : IInterceptor
{
    public void Intercept(IInvocation invocation)
    {
        OnStart();
        try
        {
            invocation.Proceed(); //The actual method call is done here
            OnSuccess();
        }
        catch(Exception ex)
        {
            OnError(ex);
        }
        finally
        {
            OnExit();
        }
    }

    protected abstract void OnStart();
    protected abstract void OnSuccess();
    protected abstract void OnError(Exception exception);
    protected abstract void OnExit();
}

This can now be bound (I won't go into the d

StackExchange Code Review Q#52668, answer score: 8