Encapsulating common Try-Catch code. Is this a known pattern? Is it good or bad?

Question

In an effort to reduce code duplication, I often use and have used this style to capture handling of exceptions on a boundary of an application: Given the following extension methods:

public static class FuncExtenstion
{
    [DebuggerStepThrough]
    public static Action<A> AsAction<A, T>(this Func<A, T> f)
    {
        return (a) => f(a);
    }

    [DebuggerStepThrough]
    public static Func<bool> AsFunc(this Action a)
    {
        return () => { a(); return true; };
    }
}

public static class IlogExtensions
{
    public static TResult TryCatchLogThrow<TResult>(this ILog logger, Func<TResult> f)
    {
        try
        {
            return f();
        }
        catch (Exception ex)
        {
            logger.Error(ex.Message, ex);
            throw;
        }
    }

    public static void TryCatchLogThrow(this ILog logger, Action f)
    {
        logger.TryCatchLogThrow(f.AsFunc());
    }

    public static TResult TryCatchLog<TResult>(this ILog logger, Func<TResult> f)
    {
        try
        {
            return f();
        }
        catch (Exception ex)
        {
            logger.Error(ex.Message, ex);
            return default(TResult);
        }
    }

    public static void TryCatchLog(this ILog logger, Action f)
    {
        logger.TryCatchLog(f.AsFunc());
    }
}

In the code I will use these (e.g. in a Windows/WCF/Web Service) to either silently catch the error and let the code continue to run (used in a polling service) or to at least log the error locally on the server and then rethrow the full exception again. I have used several other variations where the exception gets wrapped in more 'user friendly' exceptions or in WCF FaultExceptions

So typically this code is used as follows:

    Response ISomeService.SomeOperation(Request request)
    {
        return _logger.TryCatchLogThrow(() => _domainImplementation.SomeOperation(request));
    }

    OtherResponse ISomeService.OtherOperation(OtherRequest request)
    {
        return _logger.TryCatchLogThrow(() => _domainImplementation.OtherOperation(request));
    }

I have not seen this style of code anywhere else. So I was wondering if there is an other pattern I should be using, or if this is ok to use.

Answer 1

This looks like the Execute Around pattern in its static syntax. A thunk is passed around, and then invoked at the appropriate moment.

The intent is however a bit different. In true execute around, the is something done before and something done after a certain action/function/block. Catching exceptions "feels" a bit different.

Answer 2

The general advice for years has been "Do not catch general exception types". This has been debated in many places, including on Stack Overflow. Basically, the advice only holds true if you don't rethrow the exception. Since you are, at least in one instance, it's not a big deal. The TryCatchLog is a little more dangerous, since you're not rethrowing, but merely logging. That opens the potential for the application to be in a bad state.

Answer 3

Aspect-Oriented Programming

Rather than wrapping methods (which still is kind of boiler-plate code, which you wanted to get rid of in the first place), use AOP. AOP allows you to write and configure the wrapper functionality at a single place. Known frameworks for AOP are:

• ContextBoundObject
• Autofac
• And many others..

---

Exception hiding

The problem with code like this..

try
{
    return f();
}
catch (Exception ex)
{
    logger.Error(ex.Message, ex); // <- could throw error that hides 'ex'
    throw;
}

..is that it can hide the original exception ex. There are some situations that could invoke this unwanted behavior, for instance:

• if ex is OutOfMemoryException, chances are logger.Error would throw its own instance of OutOfMemoryException, hiding the original error and stack trace.
• if logger throws an error logging the original error (however, most logger frameworks tend to have fallback after fallback to try to mitigate an error propagating up)

At least, make the code a bit more robust. This is one way. Another is to build an AggregateException containing both errors, but this introduces new potential issues (what if we fail building the error..) that need to be addressed.

try
{
    return f();
}
catch (Exception ex)
{
    try
    {
        logger.Error(ex.Message, ex); // <- you will not hide my error!
    }
    catch
    { 
        // we did what we could..
    }
    throw;
}