Decoration Freak, or OCP in action?

Question

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

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

Implemented like this:

/// <summary>
/// An object that can read a code file and return its content.
/// </summary>
public class CodeFileReader : ICodeFileReader
{
    /// <summary>
    /// Reads and returns the contents of the specified code file.
    /// </summary>
    /// <param name="path">The code file to read.</param>
    /// <returns>Returns all lines in the code file.</returns>
    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<ICodeFileReader>, 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)
    {
        return _reader.ReadFile(path);
    }
}

From this base class, specialized decorators can be derived.

I have one that validates the file's header:

public class FileHeaderValidatorCodeFileReader : CodeFileReaderDecorator
{
    public static readonly int MinimumValidLineCount = 10;

    public FileHeaderValidatorCodeFileReader(ICodeFileReader reader) 
        : base(reader) 
    {
    }

    public override string[] ReadFile(string path)
    {
        var content = base.Reader.ReadFile(path);

        // this is what a class module file header should look like:

        /*  VERSION 1.0 CLASS
            BEGIN
              MultiUse = -1  'True
              Persistable = 0  'NotPersistable
              DataBindingBehavior = 0  'vbNone
              DataSourceBehavior  = 0  'vbNone
              MTSTransactionMode  = 0  'NotAnMTSObject
            END
            Attribute VB_Name = "SqlCommand"
            Attribute VB_GlobalNameSpace = False
            Attribute VB_Creatable = True
            Attribute VB_PredeclaredId = False
            Attribute VB_Exposed = True
         */

        var isValid = content.Length >= MinimumValidLineCount
                    && content[0].StartsWith("VERSION 1.0 ")
                    && content[1].StartsWith("BEGIN");

        if (isValid && content.Any(line => line.StartsWith("Attribute VB_Name = ")))
        {
            return content;
        }
        else
        {
            throw new InvalidFileHeaderException();
        }
    }

    public class InvalidFileHeaderException : FormatException
    {
    }
}

And another that logs exceptions (at a configurable log level) before rethrowing them:

public class ExceptionLoggerCodeFileReader : CodeFileReaderDecorator
{
    public enum LogLevel
    {
        Trace,
        Debug,
        Info,
        Warn,
        Error,
        Fatal
    }

    private readonly ILogger _logger;
    private readonly LogLevel _level;

    private readonly Action<Exception> _log;

    public ExceptionLoggerCodeFileReader(ICodeFileReader reader, ILogger logger, LogLevel level)
        : base(reader)
    {
        _logger = logger;
        _level = level;

        var actions = new Dictionary<LogLevel, Action<Exception>>
                    {
                        { LogLevel.Trace,  new Action<Exception>((exception) => _logger.TraceException(exception.Message, exception))},
                        { LogLevel.Debug,  new Action<Exception>((exception) => _logger.DebugException(exception.Message, exception))},
                        { LogLevel.Info,  new Action<Exception>((exception) => _logger.InfoException(exception.Message, exception))},
                        { LogLevel.Warn,  new Action<Exception>((exception) => _logger.WarnException(exception.Message, exception))},
                        { LogLevel.Error,  new Action<Exception>((exception) => _logger.ErrorException(exception.Message, exception))},
                        { LogLevel.Fatal,  new Action<Exception>((exception) => _logger.FatalException(exception.Message, exception))}
                    };
        _log = actions[level];
    }

    public override string[] ReadFile(string path)
    {
        try
        {
            return base.Reader.ReadFile(path);
        }
        catch (Exception exception)
        {
            _log(exception);
            throw;
        }
    }
}

I see focused code that has very little reasons to change, and to implement new behavior to an ICodeFileReader implementation, there's nothing else than the code for that behavior to implement, and then the code for the corresponding unit tests.

I'd like a peer review of the implementations I have here - I'm especially interested in the base.Reader.ReadFile(path) notation, versus this.Reader.ReadFile(path), to refer to a protected member in the base class. Which is more readable, and why? Is there a better way to implement this?

Also I'm wondering, am I being a decoration freak here, or is this just the Open/Closed Principle at play? Would it be simpler to just drop all those decorators and implement the functionality directly in CodeFileReader? Is it possible to do that and to follow OCP?

Answer 1

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 details of how) to any class/method, so that when the class is retrieved from the container and the method called, that interceptor is used. You don't need to use that exact format of course, but those are the basic places at which you can perform some action like logging.

The ExceptionLoggerCodeFileReader can now be replaced with an ExceptionInterceptor, which would essentially be an implementation of the OnError method above. Notice that it's completely decoupled from the actual method that's being called- no knowledge of the method, class or interface corresponding to the invocation. So now this cross-cutting concern, instead of having to be sprayed all over your project in repetitive decorators, has its own single class which is appropriate for what it's trying to do. This is a specific example, but because of the decorator's coupling to the interface, AOP is pretty much always preferred over the decorator pattern for cross-cutting concerns.

The Alternatives - Part 2: Perspective Shift (FileHeaderValidatorCodeFileReader)

That's all well and good for exception logging, but file header validation certainly isn't a cross-cutting concern. And to work out how to best deal with responsibilities like that, I'm going to suggest a bit of a perspective shift.

To start, let's look at the only public member on CodeFileReader:

public string[] ReadFile(string path)
{
    return File.ReadAllLines(path);
}

And here's the initial shift in perspective: don't call that a partially-implemented CodeFileReader. Call it a fully implemented FileReader.

So what does that achieve? Well, it means the class you just wrote is definitely going to adhere to the OCP- you can be as confident as it's ever reasonable to expect to be that you're not going to have to go in and change- or even decorate- that class. Fine. But it also seems like an exercise in rug-sweeping: what happens when you need some code somewhere to handle additional responsibilities relating to reading code files?

Well, here's where the second perspective shift comes in. You should not be trying to create a high-level design in advance to meet all possible future requirements. As an example, let's take the FileHeaderValidatorCodeFileReader again. Now imagine I gave you the following new requirement:

Entire source control repositories will be processed by this application, so it will be the application's responsibility to recognise and avoid attempting to process files which are not VBA.

(Potentially the best way to deal with this would be by looking at file extension, but since this is only supposed to be an illustrative example and since we're already talking about a file header validator, I'll work with the somewhat contrived assumption that you're additionally required to treat header validation as the only reliable way of determining whether or not a file is valid code which needs to be processed)

So, this causes you two big problems regarding the OCP:

• Your FileHeaderValidatorCodeFileReader treats an invalid header as exceptional, and this can't be fixed without modifications to the class itself
• This will require changes that can't be accomodated without also making a modification to the consumer of this class. Your main options are:
  • Change your method signature to a "Try..." pattern, like bool TryReadFile(string path, out string[] fileLines);
  • Keep the existing method signature the same but add a CheckValid method.
  • Return null for invalid files.

The relative merits of those options don't really matter- the first two would mean an interface change and the last would mean any consuming code would need to add null checking logic. None of them comply with OCP.

In trying to give yourself better adherence to OCP, with this particular example requirement you'd actually have locked yourself into a design that would need bigger modifications. That particular requirement was just the first one off the top of my head, it may not have been realistic, or perhaps you can think of workarounds, but in general you should work on the assumption that developers are bad at guessing future requirements.

So once you realise that header validation of some kind was a requirement, what should you write? Something like:

public class CodeFileHeaderValidator : IFileValidator
{
    public bool IsValid(string[] fileContent)
    {
        //Your actual implementation goes here
    }
}

And at that point, you're done. Again, what you've written now definitely adheres to OCP- there's no situation in which you'll have to change that class. Unknown requirements are kept in the realm of code you haven't written yet, and design you're not locked into:

• Will there be more validation rules that should be pulled together into a compound validator (like in your answer)?
• Will a file that fails validation be an exceptional situation or not?
• Will this always be called at the same time as the file is first loaded, or might it be required at other times?
• Will the file contents always be read from disk or may there be other data sources like a source control API or database which provide code files which require the same validation?
• Will this- and other validation rules if there are any- always be required or will there be some condition determining which validation rules are relevant, and if the latter, which class's responsibility will it be to decide which conditions are relevant?

The decorator pattern has opinions about almost all of those questions (and probably more I haven't thought of!), whereas writing a minimal implementation, decoupled from any interface like ICodeFileReader does not. That's adherence to OCP- writing code that won't have to change no matter what the answers to those above questions turn out to be.

Pulling it together

At some point, you will have to write code that handles all the responsibilities associated with reading code files. Hopefully the examples in the previous section have convinced you that trying to come up with a design which can handle arbitrary changes in responsibilities is impossible. But by having a design that:

• is loosely coupled, with nothing depending on interfaces or classes it shouldn't have to care about,
• uses dependency injection where appropriate to allow concrete implementations to be swapped without requiring modification to the classes using them,
• makes as few assumptions about unknown requirements as possible, and when it does make assumptions, chooses sensible and minimal over trying to be all-encompasing,
• adheres to the SRP

You will minimize the likelihood of having to make changes, and minimize the magnitude and risk of the changes you have to make.

With just the information from the OP, I wouldn't create an actual CodeFileReader class, because all I know is that code files need to be read and they need to be validated, and without answers to some of the previous questions, I would have no idea whether it's appropriate to group those two pieces of functionality into a file reader class. Potentially it might turn out to be more appropriate for the validator to be used by the code that calls IFileReader.ReadFile, rather than an actual IFileReader implementation. But if I did find out more and that seemed appropriate, and it turned out that invalid files were exceptional, the design would probably be:

public class ValidatingFileReader : FileReader
{
    private IFileValidator _fileValidator;

    public ValidatingFileReader(IFileValidator fileValidator)
    {
        _fileValidator = fileValidator;
    }

    public override string[] ReadFile(string path)
    {
        var content = base.ReadFile(path);
        if(!_fileValidator.IsValid(content))
            //Throw some kind of exception here
        return content;
    }
}

(Logging would be left to interceptors as described previously.)

Is it impossible that this class, or code consuming it, would have to change? No. But by calling it a ValidatingFileReader rather than a CodeFileReader and adhering to the SRP, I've made it more likely that a new requirement would be an extension of the class rather than a modification. By implementing an interface, I've made it more likely that if this class needs to be extended or swapped out, the consuming code won't need to be modified. By completely decoupling the validation implementation from this class, I've ensured that any change in validation rules will almost certainly not impact this class. And I believe that that's about as close as you should reasonably strive to get to ideally OCP-compliant code.

Answer 2

The FileHeaderValidatorCodeFileReader rules are arbitrary, and a potential failure point. Hard-coding the validation rules into a bunch of conditions isn't very maintenance-friendly.

Create an abstraction to represent a validation rule:

public interface IValidationRule
{
    string Name { get; }
    string Message { get; }
    bool Evaluate(string[] content);
}

And then receive an IEnumerable<IValidationRule> constructor parameter:

private readonly IEnumerable<IValidationRule> _validationRules;

public FileHeaderValidatorCodeFileReader(ICodeFileReader reader, 
                                         IEnumerable<IValidationRule> validationRules)
    : base(reader)
{
    _validationRules = validationRules;
}

Now the validation code looks like this:

foreach (var rule in _validationRules)
{
    if (!rule.Evaluate(content))
    {
        throw new InvalidFileHeaderException(rule);
    }
}

Or with a bit of LINQ, like this:

foreach (var failedRule in _validationRules.Where(rule => !rule.Evaluate(content)))
{
    throw new InvalidFileHeaderException(failedRule);
}

And now you just need a test that validates the the code calls .Evaluate(content) on each rule you give it, and a test to validate that the code throws when a rule fails.

The rules can be configured at the application's entry point / composition root, and modified/refined (/fixed?) without needing to recompile the decorator.

---

The ExceptionLoggerCodeFileReader is way overboard. Consider:

public class ExceptionLoggerCodeFileReader : CodeFileReaderDecorator
{
    private readonly ILogger _logger;
    private readonly string _message;

    public ExceptionLoggerCodeFileReader(ICodeFileReader reader, ILogger logger, string message)
        : base(reader)
    {
        _logger = logger;
        _message = message;
    }

    public override string[] ReadFile(string path)
    {
        try
        {
            return base.Reader.ReadFile(path);
        }
        catch (Exception exception)
        {
            _logger.ErrorException(_message, exception);
            throw;
        }
    }
}

The only immediate reason this code might need to change, is to replace _logger.ErrorException with a method that produces a log entry at another log level. So be it, it's a trivial change.

Why take a string message constructor parameter? Because using exception.Message in the log entry will likely produce redundant logs when an exception is configured with ToString - the log message will be repeated in the string representation of the exception. And move the concern of coming up with a log message for the specified path, to the calling code, who already knows about the path.

And it's simple as can be, and now you can write a ShouldLogExceptionAtErrorLevel test that will break when _logger.ErrorException is changed for something else, and a ShouldRethrow test that will break if the throw; instruction is removed.

---

The Decorator Pattern is a nice pattern, indeed OCP-friendly. However one needs to consider this:

Applicability

Use Decorator

• to add responsibilities to individual objects dynamically and transparently, that is, without affecting other objects.
• for responsibilities that can be withdrawn.
• when extension by subclassing is impractical. Sometimes a large number of independent extensions are possible and would produce and explosion of subclasses to support every combination. Or a class definition may be hidden or otherwise unavailable for subclassing.

_{Design Patterns, Elements of Reusable Object-Oriented Software, Structural Patterns / Decorator, p.177}

Dynamically adding responsibilities sounds like overkill here, but considering the dependencies are being injected at run-time through IoC, this merely boils down to being able to configure which responsibilities we're giving our ICodeFileReader implementation - if the responsibilities need to change, all that needs to change is the IoC configuration.

If the composition root is in another assembly, then the only assembly that needs a new build is that one, assuming responsibilities are being withdrawn - maybe we no longer want to log exceptions thrown by that class; theres' nothing to change in the implementation, we just tell the IoC container to skip the ExceptionLoggerCodeFileReader decorator when resolving an ICodeFileReader implementation.

Is decorator a good design decision in this case? Consider this:

public class CodeFileReader : ICodeFileReader
{
    private readonly ILogger _logger;
    private readonly string _message;
 
    private readonly IEnumerable<IValidationRule> _validationRules;

    public CodeFileReader(ILogger logger, string message, 
                          IEnumerable<IValidationRule> validationRules)
    {
        _logger = logger;
        _message = message;
        _validationRules = validationRules;
    }

    public string[] ReadFile(string path)
    {
        try
        {
            var content = File.ReadAllLines(path);

            foreach (var rule in _validationRules)
            {
                if (!rule.Evaluate(content))
                {
                    throw new InvalidFileHeaderException(rule);
                }
            }
            
            return content;
        }
        catch (Exception exception)
        {
            _logger.ErrorException(_message, exception);
            throw;
        }
    }
}

This implementation would do exactly the same thing as the bunch of decorators. From a pragmatic point of view, it could be considered a "simpler" approach.

From a maintainability point of view though, there's a problem:

• As more concerns and responsibilities are added, new dependencies are either added to the constructor for constructor injection, or tightly coupled and new'd up directly. As the class grows, its constructor becomes a mess of unrelated parameters. So you start newing things up instead, and introduce coupling and make unit tests depend on things they can't control.
• 3 levels of indentation. Both decorator implementations in the OP only needed only 1.
• I suppose the code for verifying the file's extension could be inserted before the line where content is assigned.
• The intent of the code is diluted, lost in a bunch of arbitrary checks and features.

I'm biased, but I think chosing a Decorator Pattern here has made unit testing much easier, produced cleaner code with much higher cohesion and lower coupling (the "simple" code is tied to a System.IO.File static method), and increased readability.