Single Responsibility, Api Methods, Error Logging, and Helper Classes

Question

I'm getting in a heated debate about the Single Responsibility Principle and some code that I wrote. I feel strongly that I am right and would like some unbiased feedback.

Basically, we have decided to log a certain way in our api methods. Since this code is exactly the same across all api methods I wrote some code to wrap it up in a class I'm calling MethodSupport. I think I should note, eventually we plan to move to using an attribute that will add logging and error handling to our api methods.

public class MethodSupport
{
    public string ClassName { get; private set; }
    public string MethodName { get; private set; }
    public Dictionary<string, string> Parameters { get; private set; }

    public MethodSupport (string className, string methodName, Dictionary<string, string> parameters)
    {
        this.ClassName = className;
        this.MethodName = methodName;
        this.Parameters = parameters;
    }

    public string GetSignature() 
    {
        var parametersCsv = String.Join(", ", this.Parameters);
        var message = $"{this.ClassName}.{this.MethodName}({parametersCsv})";
        return message;
    }

    public void LogBegin()
    {
        var message = this.GetSignature() + " : Begin";
        Logger.Debug(message);
    }

    public void LogEnd()
    {
        var message = this.GetSignature() + " : End";
        Logger.Debug(message);
    }

    public HttpResponseMessage RunLogic(HttpRequestMessage request, Func<HttResponseMessage> func)
    {
        this.LogBegin();

        HttpResponseMessage response;
        try
        {
            response = func();
        }
        catch (Exception e)
        {
            response = ExceptionHelper.HandleException(request, this, e);
        }

        this.LogEnd();
        return response;
    }
}

Used like so...

public class TestController : ApiController
{
    public HttpResponseMessage Get(int id)
    {
        var methodSupport = new MethodSupport(this.ToString(), nameof(Get), new Dictionary<string, string> { { nameof(id), id.GetType() } });
        var result = methodSupport.RunLogic(Request, () => 
        {
            //logic for the api method...
        });
        return result;
    }
}

So, I'm open to a full discussion on the validity of this code. That being said, the specific response from my coworker is that MethodSupport has two responsibilities. Logging and running logic. My response is, logging is not a responsibility of this class because we could change how we log and it wouldn't effect this class (unless we change contracts/interface). What are your thoughts?

I should add, he is our architect and wants me to delete the code leaving us with repeating everything that MethodSupport does for all our api methods. In his defense, there is a plan to redo the application but we haven't even started on that.

Answer 1

As for the architect's objection, I can't say I disagree, that class is doing a whole bunch of different stuff: logging, exception handling, and potentially other pre- and post-processing hooks (a name like MethodSupport, which could mean pretty much anything to anyone other than you, will encourage developers to stick whatever they want in there). In its current incarnation, if I want logging, I also have to have special exception handling, and vice versa-- I can't have one or the other. That isn't consistent with SRP.

Also... maybe I just have to get accustomed to it... but asking everyone to stick the main logic of the service into a lambda expression seems rather invasive. Maybe it's just me.

Here is a sketch which illustrates a solution that looks similar but is less invasive and maintains SRP:

using (new TimedTransaction())
using (new ExceptionScope(this))
{
    //Main logic goes here.  No lambda BTW :)
}

Logging of start and end times occur in TimedTransaction::ctor and Dispose, respectively. You can pass the metadata in the constructor, but wouldn't it be nice if you could sniff the class and method name from StackTrace during construction instead? Less typing for everyone else :)

public TimedTransaction()
{
    var trace = new StackTrace();
    _method = trace.GetFrame(1).GetMethod();
}

public string GetSignature() 
{
    var message = $"{_method.DeclaringType.Name}.{_method.Name}";
    return message;
}

The constructor for ExceptionScope subscribes to the controller's OnException event and unsubscribes when disposed; if an exception can be handled, it sets ExceptionHandled.

Still, people are going to need to remember to add these for each and every method. Filter attributes would be better. Or you could override ExecuteCore and call what you need before and after the method runs.

Answer 2

As far as I can tell, your co-worker is simply asking you to split up the core behaviour of your MethodSupport class so that it is not doing logging, however I can't see any cause to repeat your logging or violate DRY.

Here's a possible suggestion for splitting up MethodSupport into two different classes:

Firstly, consider creating an interface for your MethodSupport with the intention of injecting the MethodSupport class as a dependency:

public interface IMethodSupport 
{
    string ClassName { get; }
    string MethodName { get; }
    string Dictionary<string, string> Parameters { get; }
    string GetSignature();
    HttpResponseMessage RunLogic(HttpRequestMessage request, Func<HttpResponseMessage> func);
}

With an implementation which has no logging whatsoever:

public class MethodSupport : IMethodSupport
{
    public string ClassName { get; private set; }
    public string MethodName { get; private set; }
    public Dictionary<string, string> Parameters { get; private set; }

    public MethodSupport (string className, string methodName, Dictionary<string, string> parameters)
    {
        this.ClassName = className;
        this.MethodName = methodName;
        this.Parameters = parameters;
    }

    public string GetSignature() 
    {
        var parametersCsv = String.Join(", ", this.Parameters);
        var message = $"{this.ClassName}.{this.MethodName}({parametersCsv})";
        return message;
    }

    public HttpResponseMessage RunLogic(HttpRequestMessage request, Func<HttResponseMessage> func)
    {
        HttpResponseMessage response;

        try
        {
            response = func();
        }
        catch (Exception e)
        {
            response = ExceptionHelper.HandleException(request, this, e);
        }

    return response;
    }
}

Secondly, consider a class which performs the logging and accepts IMethodSupport as a dependency, so it may support the same behaviour to the outside world without explicit knowledge of the implementation of MethodSupport; ensuring the behaviour of both remain independent of each other

public class LoggingMethodSupport
{
    private readonly IMethodSupport _methodSupport;


    public LoggingMethodSupport(IMethodSupport methodSupport)
    {
        _methodSupport = methodSupport;
    }

    public void LogBegin()
    {
        var message = _methodSupport.GetSignature() + " : Begin";
        Logger.Debug(message);
    }

    public void LogEnd()
    {
        var message = _methodSupport.GetSignature() + " : End";
        Logger.Debug(message);
    }

    public HttpResponseMessage RunLogic(HttpRequestMessage request, Func<HttResponseMessage> func)
    {
        this.LogBegin();
        var response = _methodSupport.RunLogic(request, func);
        this.LogEnd();
        return response;
    }    
}

Lastly, you can update your usage of this class as follows:

public class TestController : ApiController
{
    public HttpResponseMessage Get(int id)
    {
        // TODO - Use some IOC container to do this and decouple from the controller...?
        var methodSupport = new MethodSupport(this.ToString(), nameof(Get), new Dictionary<string, string> { { nameof(id), id.GetType() } });
        var loggingMethodSupport = new LoggingMethodSupport(methodSupport);
        var result = loggingMethodSupport.RunLogic(Request, () => 
        {
            //logic for the api method...
        });
        return result;
    }
}

This would seem to me to adhere more closely to SRP as your co-worker has suggested, without introducing any repetition (no violation of DRY needed).

Is this overkill? Perhaps. It's certainly possible to take the SRP and other SOLID principles 'a step too far'; however as developers it is up to all of us to use our own sound judgement as to where to draw the line - there might be perfectly good reasons for doing this, specific to your project and development team, which might be a matter for debate within your team. Other projects and other teams may decide that adhering strictly to SOLID is less important to them. There are no universally correct answers.

Answer 3

It sounds like you guys are splitting hairs a bit. Single Responsibility is a principle, not a literal mandate. Yet, good to listen to your Architect. Someday you'll be the Architect, and you'll want developers to follow your vision.

One thought is to just rename RunLogic() to RunAndLogLogic(). Then the method at least is doing exactly what it says.

To get more elegant about it, the method could take an optional OnStart delegate (or collection thereof) and an optional OnEnd delegate (or collection thereof). You would pass a delegate that logs a begin message and a delegate that logs an end message.

The Attribute idea that you mentioned will still require code to implement the Attribute, I think; violating Single Responsibility. UNLESS you use a logging Aspect (I use PostSharp). Then you're golden.