3

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.

  • 5
    I would be much more concerned about the fact that you have to call this method, along with all it's parameters for each method in each controller. Something like this should be handled automatically by the infrastructure of the application. – Euphoric Feb 8 '17 at 20:25
  • Yeah. Good point. – christo8989 Feb 8 '17 at 21:38
  • 1
    Logging is a cross-cutting concern. As such it's best implemented using Ambient Context, rather than newing it up in every class. – Eternal21 Feb 9 '17 at 20:17
4

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.

  • I'm not sure how StackTrace works. That does sound like it could be nice. – christo8989 Feb 8 '17 at 21:24
  • This is how I feel. Naming can be changed. Maybe @Ben Cottrell's advice to name it LoggingMethodSupport is better. The exception handling is actually not handled by MethodSupport, it's handled by ExceptionHelper. We could also move the try catch out of RunLogic and put it in the api methods. Also, you are giving an alternative solution which seems valid (idk if I completely follow it but that's fine). This is just arguing if it really doesn't abide by single responsibility or if it just needs a name change. – christo8989 Feb 8 '17 at 21:35
  • That's really cool. Does it use Reflection? – christo8989 Feb 8 '17 at 21:49
  • 1
    It's inSystem.Diagnostics, not System.Reflection. ReflectionPermission is not needed. – John Wu Feb 8 '17 at 22:03
3

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.

  • I am perfectly fine with this response. I think the abstraction is a little low for what we are trying to achieve but it still solves the underlying problem. – christo8989 Feb 8 '17 at 21:18
  • On a side note, is there ever a reason to allow for duplicate code (at least beyond the rule of 3)? – christo8989 Feb 8 '17 at 21:18
  • 1
    @christo8989 the usual reasons I can think of would typically be a nearby deadline, an angry customer who needs something fixed "yesterday", a bit of unstable/hacky code which has a high risk of breaking if you change it (i.e. where the risk of refactoring is overwhelmingly high and you "can't afford" to avoid duplication), In short I would say "yes" there are reasons to duplicate code, but I cannot think of any 'good' or 'nice' reasons. – Ben Cottrell Feb 8 '17 at 21:34
2

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.