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.