Decorator pattern diagram

I am working with developers fascinated by the Decorator pattern. One of the things that one of them commonly says is "Decorator is one of the best ways of maintaining the Single Responsibility Principle", and I agree with him. Altough I knew this Pattern before, I never used it with so much intensity than now, that I am on a new project at my company. I even started to think why I did not used it more before.

I decided to mimic my friend and become a evangelist of the pattern at my team. On my code reviews, being used to find situations where it could be applied, I quickly suggested to developers where it would be fit and make things easier to maintain on the future. And as with every idea, we have people that goes with it, and people that goes against it.

I believe that at software development, no rule is unquestionable. An evangelist can change his ideas: is not about pushing this pattern, but discussing with others why he found it useful, and showing, in practice, why it is. So, I decided to hear developers against the use of the pattern in their code reviews, and I decided to show my point of view about it.

In one of those reviews, after letting my suggestion about using this pattern to extract logging behavior of an HttpClient, one of the answers was something like: "I guess you have a point here, sounds nice being able to test this class without worrying about logging. But, on the other side, this class is tiny. It does simple tasks, and the code of it does not even fill a screen. Is it worth making things more complex creating another class and composition just to extract this small behavior from it?"

To make things clearer, I am showing here a class that is very similar the one we were working on. Let's work on this class as it was the original one.

public class OnlineValidator : IOnlineValidator
{
    public readonly IConfigurationService ConfigurationService;
    public readonly ILogService LogService;
    public readonly HttpClient HttpClient;

    public OnlineValidator(
        IConfigurationService configurationService, 
        ILogService logService,
        HttpClient httpClient)
    {
        if (configurationService == null) 
            throw new ArgumentNullException(nameof(configurationService));

        if (logService == null) 
            throw new ArgumentNullException(nameof(logService));

        if (httpClient == null) 
            throw new ArgumentNullException(nameof(httpClient));

        HttpClient = httpClient;
        ConfigurationService = configurationService;
        LogService = logService;
    }

    public bool IsValid(string value)
    {
        try
        {
            LogService.LogInfo($"[VALIDATION STARTED] Value: {value}");

            if (string.IsNullOrEmpty(value)) return false; 

            var request = CreateRequest(value);
            var httpResponse = HttpClient.SendAsync(request).Result;

            if (httpResponse.IsSuccessStatusCode)
            {
                var content = httpResponse.Content.ReadAsStringAsync();

                LogService.LogInfo(
                    $"[VALIDATION ENDED] Result: {content.Result}");

                var jsonResponse = new { Success = false };

                var result = JsonConvert.DeserializeAnonymousType(
                    content.Result, 
                    jsonResponse);

                return result.Success;
            }

            return false;
        }
        catch (Exception ex)
        {
            LogService.LogError("[VALIDATION ERROR] Failure", ex);

            return false;
        }
    }

    private HttpRequestMessage CreateRequest(string value)
    {
        var uri = ConfigurationService.GetValue("ValidationServiceUri");
        var endpoint = $"{uri}?value={value}";

        var request = new HttpRequestBuilder(
            endpoint, 
            HttpMethod.Get).Build();

        return request;
    }
}

I guess it's my time to say "you have a point here". In fact, the HttpClient was tiny, and we had no trouble reading the code and understanding what it does. The code was, with a reasonable answer, readable.

But in my opinion, in this case, the question is more complex than just "being a small enough class". As far as I know, this class is violating the single responsibility principle: making a validation of a value. OnlineValidator should not be responsible of logging the result of its validation.

And why not make it even more readable? What if we could remove all logging operations? We could make this class easier to understand and read. At the point that, once we are reading it, at every meaningful step we would be echoing in our minds: "mhm, yes, that's it". We would agree in every single step of the operation this class is meant to do, without stopping the focus on the problem to be solved.

In this code, we do a step, then we stop to say (log) "I did it, log it now.". Another couple of steps, and we do that again. It is, in a certain way, a pain in our readability and the process. We have to stop our focus on our task, and do something necessary for our infrastructure (not that this is not important).

So, the question is, how to focus on a task, and still do the necessary infrastructure stuff? Here is where Decorator pattern can shine. Let's write another implementation of the IOnlineValidator interface, and call it LoggedOnlineValidator:

public class LoggedOnlineValidator : IOnlineValidator
{
    public readonly IOnlineValidator EncapsulatedValidator;
    public readonly ILogService LogService;

    public LoggedOnlineValidator(
        IOnlineValidator encapsulatedValidator,
        ILogService logService)
    {
        if (encapsulatedValidator == null)
            throw new ArgumentNullException(nameof(encapsulatedValidator));

        if (logService == null)
            throw new ArgumentNullException(nameof(logService));

        EncapsulatedValidator = encapsulatedValidator;
        LogService = logService;
    }

    public bool IsValid(string value)
    {
        try
        {
            LogService.LogInfo($"[VALIDATION STARTED] Value: {value}");

            var result = encapsulatedValidator.IsValid(value);

            LogService.LogInfo($"[VALIDATION ENDED] Result: {result}");

            return result;
        }
        catch (Exception ex)
        {
            LogService.LogError("[VALIDATION ERROR] Failure", ex);

            return false;
        }
    }
}

What do we have here? Now we have a logged online validator: A validator that logs operations. Sounds like it's doing more than one thing, but in fact, is not. What it does is logs the operations of another OnlineValidator. We extracted this responsibility for an upper layer of the code, and now our real OnlineValidator can focus on its task:

public class OnlineValidator : IOnlineValidator
{
    public readonly IConfigurationService ConfigurationService;
    public readonly HttpClient HttpClient;

    public OnlineValidator(
        IConfigurationService configurationService,
        HttpClient httpClient)
    {
        if (configurationService == null) 
            throw new ArgumentNullException(nameof(configurationService));

        if (httpClient == null) 
            throw new ArgumentNullException(nameof(httpClient));

        HttpClient = httpClient;
        ConfigurationService = configurationService;
    }

    public bool IsValid(string value)
    {
        if (string.IsNullOrEmpty(value)) return false; 

        var request = CreateRequest(value);
        var httpResponse = HttpClient.SendAsync(request).Result;

        if (httpResponse.IsSuccessStatusCode)
        {
            var content = httpResponse.Content.ReadAsStringAsync();
            var jsonResponse = new { Success = false };

            var result = JsonConvert.DeserializeAnonymousType(
                content.Result, 
                jsonResponse);

            return result.Success;
        }

        return false;
    }

    private HttpRequestMessage CreateRequest(string value)
    {
        var uri = ConfigurationService.GetValue("ValidationServiceUri");
        var endpoint = $"{uri}?value={value}";

        var request = new HttpRequestBuilder(
            endpoint, 
            HttpMethod.Get).Build();

        return request;
    }
}

This is the end of my proposed solution. I believe, although we created one more class, and substantial more lines of code, that this way of solving the problem is more appropriate, if we must log all this operations as a premise of our infrastructure. Here are my principal reasons for it:

1. Single Responsibility Principle should be maintained

I am more inclined to keep simple responsibility of a class, even if by doing that, I create more lines of code. For me, it's better to read a class and know exactly that it does one thing, and only one thing. If more operations are necessary, encapsulate this class in another class, who is responsible for one of those extra operations. Create other classes for every new operation, and keep doing it. Then, at our DI composition, build this hierarchy through Decoration.

kernel.Bind().ToMethod(context =>
{
    var logService = context.Kernel.Get();
    var appSettingsService = context.Kernel.Get();
    var httpClient = new HttpClient();

    return new LoggedOnlineValidator(
        new OnlineValidator(httpClient, appSettingsService),
        logService);
});

Why is this useful? We can now focus more on our DI than maintaining classes. We don't have to worry about logging when changing our OnlineValidator, this task is already covered in another class. If we need to attach another behavior on this flow, we just put more pieces into the composition, as if we were working with pieces of Lego.

2. Testability

Keeping concerns separated allow us to keep tests focused on units of work: tests that test validation of a value, and tests that covers logging operations. Our tests will be easier to maintain, by keeping them separate by those concerns.

[Theory, AutoNSubstituteData]
public void LoggedHttpClient_Should_Log_Beginning_Of_Operation(
    LoggedOnlineValidator sut)
{
    sut.EncapsulatedValidator.IsValid(It.IsAny<string>()).Returns(true);

    sut.IsValid("test");

    sut.LogService.Received().LogInfo("[VALIDATION STARTED] Value: test");
}

[Theory, AutoNSubstituteData]
public void LoggedHttpClient_Should_Log_End_Of_Operation(
    LoggedOnlineValidator sut)
{
    sut.EncapsulatedValidator.IsValid(It.IsAny<string>()).Returns(true);

    sut.IsValid("test");

    sut.LogService.Received().LogInfo("[VALIDATION ENDED] Result: true");
}

[Theory, AutoNSubstituteData]
public void LoggedHttpClient_Should_Log_Error_In_Operation(
    LoggedOnlineValidator sut)
{
    sut.EncapsulatedValidator.IsValid(It.IsAny<string>())
        .Returns(x => { throw new System.Exception(); });

    Assert.Throws<Exception>(() => sut.IsValid("test"));
}

3. Feature-laden classes high up in the hierarchy

I totally agree with the point mentioned by the "Gang of Four" in the GoF Design Patterns book, where they mention that an application should not pay for the features it does not use:

Decorator offers a pay-as-you-go approach to adding responsibilities. Instead of trying to support all foreseeable features in a complex, customizable class, you can define a simple class and add functionality incrementally with Decorator objects. Functionality can be composed from simple pieces. As a result, an application needn't pay for features it doesn't use. It's also easy to define new kinds of Decorators independently from the classes of objects they extend, even for unforeseen extensions. Extending a complex class tends to expose details unrelated to the responsibilities you're adding.

Even for simple tasks as logging, if we maintain simplicity by separating those features in individual classes, we are helping the future of the growing application.

Let's say that we need to e-mail the result of every operation, and keep logging it. We could implement an EmailOnlineValidator decoration, and stack it up on the composition root. And now we have another application were we only need to e-mail, but not need to log: just make the right composition. We go back to the Lego story.

4. When more code makes things easier

More code does not always mean more complexity, neither less readability. Most of the time, when a simple code violates SRP rules, it becomes a pain in some moment at the future, when the application needs more flexibility in its features. When we make more code to keep a simple set of operations by using Design Patterns, we are trying to make use of some it's benefits at the future (in this case, code reusability and code composition). Don't be afraid of writing a couple more lines of code because of that: I've seen much more lines of noisy code being written in applications because this principle were avoided in the beginning in order to have less code for classes like the one mentioned above. I have made this mistake in the past, and I am not willing to do it again.