Using a general class for execution with try/catch/finally?

Question

I find myself having a lot of this in different methods in my code:

try
{
  runABunchOfMethods();
}
catch (Exception ex)
{
  logger.Log(ex);
}

What about creating this:

public static class Executor
{

    private static ILogger logger;

    public delegate void ExecuteThis();

    static Executor()
    {
        // logger = ...GetLoggerFromIoC();
    }

    public static void Execute<T>(ExecuteThis executeThis)
        where T : Exception
    {
        try
        {
            executeThis();
        }
        catch (T ex)
        {
            // Some kind of Exception Handling Strategy...
            logger.Log(ex);
            // throw;
        }
    }

}

And just using it like this:

private void RunSomething()
{
  Method1(someClassVar);
  Method2(someOtherClassVar);
}

...

Executor.Execute<ApplicationException>(RunSomething);

Are there any downsides to this approach? (You could add Executor-methods and delegates when you want a finally and use generics for the type of Exeception you want to catch...)

Edit: Sorry for being unclear - what I was really after was some input on the general idea of trying to move the execution of code from the class in question to a more generalized class that does this. I just did a quick mock-up of a solution but in real life you would naturally use things such as exception handling strategies, abstract execution base classes with more specialized execution classes for a specific layer/part of the system. I generally create one method with the try.../runABunchOfMethods-part (this does the exception handling, with specialized exceptions) that calls the runABunchOfMethods that in turn execute a limited set of other methods "clean code" style.

I'll buy the obfuscation argument on some levels but if the whole solution/architecture takes this proposed approach a new programmer should be able to understand the pattern.

I've edited the Executor to include a generic T to allow the calling code to specify the exeception just to show how to handle a specialized exeception. In other cases you might have a bunch of catch:es depending on the what you want to do but those are special cases in the concrete subclasses I was talking about.

Answer 1

The downside would be a lack of flexibility. What if you wanted to recover from a specific exception in one of the methods being called - e.g. on a failed connection to an SMTP server you may want to store the contents of an outbound mail in your database for retry later? This the reason why you should avoid catching the general Exception type whenever possible.

Also, you lose some of the clarity of your code (in my opinion, anyway) as it is hard to see where exception handling happens.

Answer 2

The downsite is your approach of general exception handling. You should not catch the baseclass Exception without a strong reason. It could hide various problems; ok, you log them, but your code doesn't know that it just ran out of memory, or the file does not exist and continue as if there was no problem at all.
The method you describe here would encourage a general exception handling.

Related:
Is it really that bad to catch a general exception?
Is this a bad practice to catch a non-specific exception such as System.Exception? Why?

EDIT (response to the OP's edit & clarification):
I am not sure what you want to achieve. Basicall you hide an exception (general or specified - it is just swallowed). Call your method Executor.Hide<ApplicationException>( RunSomething ); and it is clear what happens here. But is there any advantage in swallowing exceptions? I don't think so. Again, there can be places where you need this - but they are rare and should be chosen intentionally. The method you provide encourages to swallow exception without thinking about it.
You commented out the rethrow line (throw ex or better just throw, which preserves the stack). What do you get with this line enabled? Basically just logging. You catch an exception, log it and rethrow it to ... catch it again? Why can't you put your logging to this latter place?
Try to catch an exception only, when you are able to handle it. There you can log, too. Any good logger will be able to show you the stack trace, so that you have no loss of information.

Related:
Where to put try catch?

Answer 3

You get another level of indirection which might make your code harder to read and understand (especially for someone looking at your code for the first time).

Answer 4

If you're seeing that code in a lot of classes then you are doing something very very wrong in my book :-) At the very least you should re-throw the exception (with just "throw", not "throw(ex)"), but the whole idea of catching the base exception all the time just to log it (or indeed at any time, bar a few edge cases) is not the right approach for me.

I have no idea what kind of application you're writing, but can you not hook events such as AppDomain.UnhandledException, which fires whenever there's an unhandled exception (one that you haven't caught) in any thread, and log those? You might want to combine that with more specific catch statements that also log, if you want to log recoverable exceptions too.

Answer 5

I'll jump on the "it makes you likely to misuse exceptions by only catching at a general level" bandwagon. However you might benefit from some sort of 'exception handling delegate wrapper' in more specialized cases. For instance if you were doing lots of work with DB-related classes that always throw the same exceptions, you could have a dedicated DB-exception-wrapper.

Ultimately though, the fact that exceptions'clutter' your code is the downside to making your code safe.

Answer 6

You don't really win anything, but you lose some things:

• Readability
• Simplicity (you introduce another class, another delegate that a reader must understand)
• You can't simply step over the Executor.Execute to get to Method1(...). If you step over, you skip the whole code block in your delegate.
• ...

Answer 7

At the very begining....

You should NOT have a code like below scattered around in your code base.

try 
{ 
  runABunchOfMethods(); 
} 
catch (Exception ex) 
{ 
  logger.Log(ex); 
} 

I would suggest you should first look at the actual need of a code like this. You should be catching a very specific exception in all the cases ( unlike a generic catch all).

After that, if you still still multiple methods catching same exception then you can think of creating multiple methods in your executor which handle a specific exception type.

Answer 8

Normally the best pattern for logging exceptions in your application is to only catch exceptions at the top-level methods in your code.

• In a console application you should catch and log exceptions in Main.
• In a WinForms application you should catch and log exceptions in your event handlers.
• In a service you should catch and log exceptions then the ThreadProcs of your worker threads.
• Etc.

At almost all other points in your application you should only catch specific exceptions and then rethrow a new exception wrapping the original exception. If you don't need to wrap the original exception you don't need to catch the original exception in the first place.

Sructuring your application like this will lead to only a few places where you catch, log and "swallow" exceptions.

Answer 9

If you want to keep your objects clean, you could consider using an AOP framework like PostSharp. Then your logging of exceptions (for example) can all be handled in one place if you so desire.

EDIT:

It is possible to remove the try / catch blocks using postsharp - here is an example common exception handler that could be created in PostSharp:

[Serializable]
public class CommonExceptionHandling : OnExceptionAspect
{
    public override void OnException(MethodExecutionEventArgs eventArgs)
    {
        // can do some logging here
        // ...

        // throw the exception (out of postsharp) to where it occurred:
        eventArgs.FlowBehavior = FlowBehavior.RethrowException;                       

        // If you want to ignore the exception, you can do this:
        //eventArgs.FlowBehavior = FlowBehavior.Return;

        base.OnException(eventArgs);
    }
}

If you apply this attribute to a class, any exceptions that any methods in that class throw will then be directed through the above code.