Should logging reside within a class who's primary purpose is not logging?

Question

This is more of a theoretical question. Should logging reside within a class who's primary purpose is not logging?

Here is a simple interface for anything that will preform a calculation on a number.

public interface ICalculation { 
    public int calculate(int number); 
}

Here is an implementation of the ICalculation interface that performs a calculation and does some logging. I believe this is a very pragmatic approach. Aside from the constructor accepting something that we wouldn't normally expect to see in a calculation's domain, the inline logging is arguably non intrusive.

public class ReallyIntenseCalculation : ICalculation {
    private readonly ILogger log;

    public ReallyIntenseCalculation() : this(new DefaultLogger()) {
    }

    public ReallyIntenseCalculation(ILogger log) {
        this.log = log;
        log.Debug("Instantiated a ReallyIntenseCalculation.");
    }

    public int calculate(int number) {
        log.Debug("Some debug logging.")
        var answer = DoTheDirtyWork(number);
        log.Info(number + " resulted in " + answer);
        return answer;
    }

    private int DoTheDirtyWork(int number) {
        // crazy math happens here
        log.Debug("A little bit of granular logging sprinkled in here.");
    }
}

After removing all the logging code from ReallyIntenseCalculation, the code now has what appears to be a clear single responsibility.

public class ReallyIntenseCalculation : ICalculation {

    public int calculate(int number) {
        return DoTheDirtyWork(number);
    }

    private int DoTheDirtyWork(int number) {
        // crazy math happens here
    }
}

Ok, so we have removed ReallyIntenseCalculation's ability to log its internals. How can we find a way to externalize that functionality. Enter the decorator pattern.

By creating a class that decorates an ICalculation we can add logging back into the mix, but doing so will compromise some of the more granular logging that was taking place within ReallyIntenseCalculation's private methods.

public class CalculationLoggingDecorator : ICalculation {
    private readonly ICalculation calculation;
    private readonly ILogger log;

    public CalculationLoggingDecorator(ICalculation calculation, ILogger log) {
        this.calculation = calculation;
        this.log = log;
        log.Debug("Instantiated a CalculationLoggingDecorator using " + calculation.ToString());
    }

    public int calculate(int number) {
        log.Debug("Some debug logging.")
        var answer = calculation.calculate(number);
        log.Info(number + " resulted in " + answer);
    }
}

What are some of the other possible pros and cons of having a logging decorator?

Answer 1

I think it's debatable. A case can be made that logging is part of that single responsibility.

That said, I think you're thinking in terms of cross-cutting concerns, which is something aspect-oriented programming deals with. In fact, logging code is the canonical example for AOP. You may want to consider an AOP framework like aspect#.

The advantages of doing something like this are of course decomposability, reuse, and separation of concerns.

Answer 2

How many loggers do you have? I'd make the implementation of the calculator's constructor privately instantiate a logger instance, using a Logger singleton or Logger factory method (so that logging or not logging isn't part of the calculator's public API, but is part of the calculator's implementation).

Answer 3

I agree with Jason that this is more of a crosscutting concern.

A function should really do only one function, as this makes the code more legible, and it becomes easier to test. For example, if my unit test is failing as the log file is full, that will be confusing, as I don't test the logging within my method.

AOP is the best choice here, and for .NET PostSharp may be the best option.

If AOP isn't a good option then you may want to use DI, and you can inject a version of the class that has logging, to test the flow, but if you are going to do this then you should ensure that the injected class just does the logging, then call the same function that doesn't have the logging, so you are putting a wrapper around your class.

public class ReallyIntenseCalculation : ICalculation {

    public int calculate(int number) {
        return DoTheDirtyWork(number);
    }

    private int DoTheDirtyWork(int number) {
        // crazy math happens here
    }
}

public class CalculationLoggingDecorator : ICalculation {
    ICalculation calculation;
    ILogger log;
    public CalculationLogging() {
        this.calculation = new ReallyIntenseCalculation() ;
        this.log = SomeLogger(...);
        log.Debug("Initialized a CalculationLoggingDecorator using " + calculation.ToString());
    }

    public int calculate(int number) {
        log.Debug("Some debug logging.")
        var answer = calculation.calculate(number);
        log.Info(number + " resulted in " + answer);
    }
}

This is similar to your decorator, but when you swap out the logging version for the non-logging version you have dropped all the excess code, and by testing the logging version you are ensuring that ReallyIntenseCalculation is being used and the methods are only defined one time.

This is more work, and AOP is preferable, but DI may be an alternative.

UPDATE: Based on comment.

If you have multiple classes extending this interface then you may have an explosion of classes, but design for that.

AOP would be the best approach, but the concept is a hard sell to some companies, than DI.

You may end up with two classes for each implementation, but your logging info is still removed from these other classes, have each class be injected in through DI, so you can have two app.config files, one with all the logging classes set up and one for production, to simplify your life. The second class just has the logging and set up info though, so it isn't too much extra work, I believe, but you have lost the fine granularity of logging.