Catch all exceptions and do...nothing?

I have just inherited code that has over 300 occurances of this:

catch(Exception ex)
{
    string s = ex.Message ;
}

The next time I come across the person that wrote this, what should I do to them?

But seriously...

This is obviously a coding horror and one of the worst things a programmer can do. Should I go through and just remove all of these and see how bad things really are when I run t开发者_如何学编程he app? How would you go about righting this wrong?

This is a WinForms application that is run internally in my organization by about 2 dozen users.

You could remove all of these catch blocks and add the following code before you launch your first form:

public static void Main(string[] args)
{
// Event handler for handling UI thread exceptions.
Application.ThreadException += 
    new ThreadExceptionEventHandler(App_UiThreadException);

// Force all Windows Forms errors to go through our handler.
// NB In .NET 4, this doesn't apply when the process state is corrupted.
Application.SetUnhandledExceptionMode(UnhandledExceptionMode.CatchException);

// Event handler for handling non-UI thread exceptions. 
AppDomain.CurrentDomain.UnhandledException += new 
    UnhandledExceptionEventHandler(App_NonUiThreadException);

// Run the application.
}

Intercepting all events

Calling SetUnhandledExceptionMode ensures that all unhandled exceptions will be intercepted regardless of application configuration settings.

Note that starting with .NET Framework 4, the events coded above and discussed below are not raised for exceptions that corrupt the state of the process, such as stack overflows or access violations - unless the event handler is security-critical and has the HandleProcessCorruptedStateExceptionsAttribute attribute.

There is an interesting blog entry on exceptions that corrupt process state.

UI thread exceptions

Handling the Application.ThreadException event intercepts unhandled exceptions on the ui thread. In your filter code, you can then log each exception. If you want to duplicate existing behaviour for ui thread exceptions, you should be able to swallow most of these.

Non-UI thread exceptions

Handling the AppDomain.UnhandledException event intercepts unhandled exceptions on non-ui threads. In your filter code, you can then log each exception. Unfortunately most non-ui thread exceptions will already be fatal by the time this event is fired, so it isn't possible to duplicate existing behaviour for non-ui thread exceptions by doing this.

I'd start with logging every time there was an exception. Then seeing how many of them were major issues. The problem with taking them all off is now you could be changing functionality. Broken functionality, but functionality your users expect.

Introduce a logging mechanism and at least log those error messages!

My suspicion is that the original author introduced these statements to facilitate debugging. It is very likely that the code was throwing an exception due to an error in to program, and the original author tried to track it down by catching it at different points.

Why this arcane approach?

1. The author didn't know that you can configure the debugger to break when an exception is thrown (CTRL+ALT+E in Visual Studio)
2. If you debug and catch an exception in an empty catch handler, it is difficult to step to a statement that allows you access to the scope of the handler and the exception message (you need to know to put the breakpoint on the closing brace)

I wouldn't be surprised if the application runs well without the no-op exception handlers; I would remove them and add a top-level logging mechanism to record unhandled exceptions as others have suggested. If you find through the logs that an unhandled exception can be handled, then you will have its call stack and know where to introduce an appropriate handler.

I would add logging to assess it in production, and rethrow in dev - to understand the issues:

LogCrazyException(ex);
#if DEBUG
    throw;
#endif

You can simply rethrow the exceptions, that way you remove it's exception swallowing function while making a minimal change to the code:

catch(Exception ex) {
  throw;
}

If you just remove all the try...catch blocks you can run into scoping problems as you remove a scope block for all the code in each try...catch. By just making them rethrow you can see where errors frequently occur, so that you can fix them. Then later on you can add proper exception handling and/or logging where needed, and remove try...catch blocks where they are not useful.

I would remove them all, except for those at the top level of an event handler. At that level, I'd display a MessageBox containing the exception information.

Well.. Perhaps the writer actually didn't want to do nothing in case of error, and since it's not possible to have a try without a catch he wrote that just to make the compiler happy. It looks strange honestly, but if the program works...

There are some cases where it is appropriate to catch an exception and do nothing. For example, when calling Control.BeginInvoke to request that a progress bar control update itself, there's an unavoidable(*) race condition if some other thread could dispose the control. While one should arguably only catch the exceptions one would expect BeginInvoke to throw, there's a philosophical argument which would say that any conditions which would cause BeginInvoke to throw may be of interest to the UI thread, but aren't really of interest to the worker thread that's trying to provide a courtesy update notification. Anything exceptions from BeginInvoke that the system will let be caught shouldn't affect the worker thread, and those that can't be caught, won't be caught. There's not really any reason why UI-related exceptions should escape from a routine whose purpose isn't to update the UI, but is instead supposed to perform business logic (with progress-bar updates as a courtesy side effect).

All that being said, 300 times sounds like too many for a Try-Catch-Ignore pattern. If there are many e.g. BeginInvoke statements wrapped in such catch blocks, they should be pulled out into some other TryBeginInvoke routine. Likewise if there are many calls to some other function which returns can't-do-anything-about exceptions. While I can imagine that there could be a total 300 calls to a few functions which return such vexing exceptions, I doubt there are 300 such functions.

(*) It would be possible to use locks to protect the BeginInvoke and Dispose methods, but that adds considerable complexity and creates additional opportunities for things to go wrong. If there existed a .net TryBeginInvoke method, that would be beautiful. Since it doesn't exist, writing such a routine to wrap a BeginInvoke with a Try-Catch-Ignore block seems the best approach.

try{

}
catch (Exception)
{
}

would prevent from warning messages