C# Anti-Pattern – Try/Catch/Log/Rethrow

cexceptionsloggingpatterns-and-practicesprogramming practices

I can see several post where importance of handling exception at central location or at process boundary been emphasized as a good practice rather than littering every code block around try/catch. I strongly believe that most of us understand the importance of it however i see people still ending up with catch-log-rethrow anti pattern mainly because to ease out troubleshooting during any exception, they want to log more context specific information (example : method parameters passed) and the way is to wrap method around try/catch/log/rethrow.

public static bool DoOperation(int num1, int num2)
{
    try
    {
        /* do some work with num1 and num2 */
    }
    catch (Exception ex)
    {
        logger.log("error occured while number 1 = {num1} and number 2 = {num2}"); 
        throw;
    }
}

Is there right way to achieve this while still maintaining exception handling good practice ? I heard of AOP framework like PostSharp for this but would like to know if there is any downside or major performance cost associated with these AOP frameworks.

Thanks!

Best Answer

The problem isn't the local catch block, the problem is the log and rethrow. Either handle the exception or wrap it with a new exception that adds additional context and throw that. Otherwise you will run into several duplicate log entries for the same exception.

The idea here is to enhance the ability to debug your application.

Example #1: Handle it

try
{
    doSomething();
}
catch (Exception e)
{
    log.Info("Couldn't do something", e);
    doSomethingElse();
}

If you handle the exception, you can easily downgrade the importance of the exception log entry and there is no reason to percolate that exception up the chain. It's already dealt with.

Handling an exception can include informing users that a problem happened, logging the event, or simply ignoring it.

NOTE: if you intentionally ignore an exception I recommend providing a comment in the empty catch clause that clearly indicates why. This lets future maintainers know that it was not a mistake or lazy programming. Example:

try
{
    context.DrawLine(x1,y1, x2,y2);
}
catch (OutOfMemoryException)
{
    // WinForms throws OutOfMemory if the figure you are attempting to
    // draw takes up less than one pixel (true story)
}

Example #2: Add additional context and throw

try
{
    doSomething(line);
}
catch (Exception e)
{
    throw new MyApplicationException(filename, line, e);
}

Adding additional context (like the line number and file name in parsing code) can help enhance the ability to debug input files--assuming the problem is there. This is kind of a special case, so re-wrapping an exception in an "ApplicationException" just to rebrand it doesn't help you debug. Make sure you add additional information.

Example #3: Don't do anything with the exception

try
{
    doSomething();
}
finally
{
   // cleanup resources but let the exception percolate
}

In this final case, you just allow the exception to leave without touching it. The exception handler at the outermost layer can handle the logging. The finally clause is used to make sure any resources needed by your method are cleaned up, but this is not the place to log that the exception was thrown.

Related Topic