Asked to fix bugs in a program, you find over 100

Question

catch(Exception ex)
{

}

What's the best way to proceed?

Rip them all out and let it crash? Add logging code? Message boxes? This is in C#.

Answer 1

The solution you should pick depends greatly on the environment you're working in.

The coding guidelines I've written and introduced to most of my customers, state explicitly that empty catch clauses without comments (exactly as you've shown in your question) may be removed without any discussion. The reason I wrote this rule is because I encounter these kind of statements all the time and they almost always hide a lot of bugs. Usually, the more code is in the try block, the more bugs they hide. Over the years I have discovered (and solved) a lot of bugs by simply removing empty catch clauses. The procedure is usually the same:

1. I remove a catch.
2. Code goes to production.
3. After some time customer complaints that the program stopped working (he got an exception).
4. I start investigating the problem and find out that this part of system never really worked, and multiple developers have tried to find bugs related to this cause, but never found the problem. Or worse, they found the problem and wrote that catch statement to ‘solve’ the problem.
5. I fix the real problem that was swept under the carpet and close multiple bug reports at once.

While this method has served me (and my customers) well these years, there is a ‘but’. For instance, one of my current customers is a medical institution and the developers were interested in using my coding guidelines. However, I insisted that they'd remove that particular rule from the guidelines. While their software has a terribly lot of those empty catch statements (well over 100) and those pesky things hide a lot of errors and byte me all the time while I work with their software; you must be very careful in these types of applications. In this scenario, I would start by adding log statements, instead of removing them. After this you can remove them slowly one-by-one when you know which types of exceptions occur and why the previous developer did add that catch statement in the first place.

In all cases, you should have a general catch statement at the top of your application (or have an exception handler in your web app) that will log any exceptions that bubble up.

Extra note, my guidelines also note that you would configure Visual Studio to break on all exceptions, by going to Debug / Exceptions... / Common Language Runtime Exceptions and check the 'Thrown' checkbox. This way you spot an exception right away.

Answer 2

One approach you could take in order to get a picture of what's going on would be to take each of the empty Exception blocks, and have each one call a method breakOnException(Exception e).

This method doesn't have to do anything except (say) log it. You can then run the program under a debugger with a breakpoint on this method and watch for the breakpoint to be hit, and then investigate the causes. Unfortunately that's a little labour-intensive.

Do you have unit tests that can be run against the code in the debugger ? It would make sense to do the above with these.

Answer 3

wow....

I would be hesitant to rip them out because odds are that would just create more "bugs". A first step would be to add logging so you know what is going on. After you have sufficient information you would be able to proceed refactoring....carefully

Unit tests would be a great way to test any refactoring you do. I would also suggest getting plenty of them in place.

Answer 4

Change the catch blocks as follows:

catch (Exception ex)
{
    Logger.Log(String.Format(
        System.Globalization.CultureInfo.InvariantCulture,
        "An exception is being eaten and not handled. "+
        "This may be hiding critical errors.\n"+
        "Exception: {0}",
        ex));
}

Answer 5

Caveat: This is general advice and quirks in your environment may make it impossible, e.g. time pressures.

Rip them all out with the exception of at the entry points to the system (main methods, service end points, top of thread call stacks, event handlers - for UI code) and add logging/error handling to the places where the exception handlers remain.

The begin a process of adding the handlers back in where required, i.e. places where exceptions should be handled lower down, with appropriate logging/error reporting.

Getting the system working again is dependent on having a good set of acceptance/regression tests to verify the system is correct after all the changes are made.

Answer 6

Obviously logging all suppressed exceptions is the first place to start - blanket suppression is useful from time to time when you need to ensure that your code gracefully degrades under all circumstances (i.e. where you don't wish to be caught out by an exception type that you failed to anticipate), but can hide an unanticipated problem that needs more handling than simply being ignored, so it is imperative that all exception handlers should at a minimum report that an exception has been suppressed, so you have a clear trail of clues to follow if an unexpected behaviour emerges.

Removing the catches is foolish, as there will be many cases where some exceptions need to be "handled" for the program to work correctly (although you may disagree with how they are handled, and there may be risks associated with this approach, the original author had a reason for writing the code this way. Ig he has not added a comment explaining his reason, then don't assume you know better than he did, especially not in a "global search and replace" manner).

However, nobody seems to have pointed out the most obvious, and quickest to implement, starting point: Go to "Debug -> Exceptions" and enable "Break when exception is thrown" for the "Common Language Runtime Exceptions" section. This will cause the debugger to break for every exception that is thrown (before the program's catch block is called to handle it), so you will be able to test the application under the debugger and determine which catch blocks are suppressing which exceptions when you try to repeat a given bug, from which you can then make a judgement call as to whether or not that particular catch block has any influence on the bug in question.