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 startnewing 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
contentis 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.
