Exceptions and error logging
At my current work I see an awful lot of code that looks like this:
public class ServiceA : IServiceA
{
private readonly ILogger _logger;
public ServiceA(ILogger logger)
{
_logger = logger;
}
public void DoIt()
{
try
{
// Do something...
}
catch(Exception e)
{
_logger.LogError(e);
throw;
}
}
}
At first blush this looks reasonable: it has constructor-based dependency injection, an abstraction, error logging… What could my issue be?
My issue is with the error logging. It may seem fairly innocuous here, but the point is that the class is cluttered up with logging code which makes it
- harder to read
- more error prone (the more code you have, the greater the risk of bugs)
- almost completely redundant
Why is it redundant? Because pretty much all applications we write will have a global error handler that can do the logging for us. After all, the catch block in this example (and this is a necessary feature to make this code a target of my point) *doesn’t actually handle the error*. It simply logs the error and then rethrows. I don’t think that it should be this class’s responsibility to do that; I argue that it violates the Single Responsibility Principle.
But isn’t logging is a cross-cutting concern?
Is it? I would argue not. The only reason this class needs to do anything at all is that it might be useful to add some more information beyond that one normally gets in an exception. For example:
public void ProcessInvoice(int invoiceId)
{
try
{
// Try to process the invoice...
}
catch(Exception e)
{
_logger.LogError(e, "ProcessInvoice failed for invoiceId = " + invoiceId);
throw;
}
}
This is a valid case of catching an exception, logging down some useful data, and then rethrowing. But, if we don’t log the exception here, then how can we make a note of this useful data? This is where the aptly-named Data property on System.Exception comes in handy.
Exception.Data is of type IDictionary and “represents a nongeneric collection of key/value pairs”, so it behaves much like a Dictionary<Object, Object>, with which we are all probably more familiar. This collection is intended for the end user to put whatever they like in there, and I would argue that it’s perfect for storing down any useful information you have (the arguments that were passed into your method, for example). That information will then make its way all the way up to your global exception handler and will then be available for your logging system to persist to whatever store you choose. It might look something like this:
public class ServiceA : IServiceA
{
public ServiceA()
{
}
public void ProcessInvoice(int invoiceId)
{
try
{
// Try to process the invoice...
}
catch(Exception e)
{
e.Data["invoiceId"] = invoiceId;
throw;
}
}
}
So now we have not lost any information, but you’ll notice that ServiceA no longer contains any logging code, and therefore has no dependency on a logging framework (or even an abstraction thereof). What’s even better is that we can automate this process. Using a simple AOP framework such as Fody, we could write a plugin that, by marking methods with an attribute, automatically injects the try/catch and e.Data assignment into your method at build time. That means that this boilerplate would disappear, and all that would be left is:
public class ServiceA : IServiceA
{
public ServiceA()
{
}
[SaveParametersToExceptionData]
public void ProcessInvoice(int invoiceId)
{
// Try to process the invoice...
}
}
That’s it. I am going to write this Fody plugin myself; you can check back here on this blog for details on the Github or BitBucket repository and follow its progress. See you soon.
Richiban 