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

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

It partly depends on how aggressive you can be. Is this app internal or external? Will your changes be deployed on live systems soon? Do you have specific bugs to fix, or is it just deemed to be a disaster?

To reduce the bug count as quickly as possible, but with the most risk of serious damage, just remove all the catch blocks and let exceptions bubble up. For a much more delicate approach, just add logging to start with.

You should also talk to whoever wrote the app to start with, if possible. Try to find out why they've got so many exception-swallowing blocks. Do they really understand exceptions at all? A certain amount of tact is necessary here, I suspect :)

Next stop: unit tests...

Answer 2

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 3

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 4

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 5

Fix the bugs that you were set out to do, and report the exception swallowing blocks as new, critical, bugs.

Answer 6

The first intermediate goal is to get a good picture of what exceptions are being ignored and where; for that purpose, you can simply add logging code to each of those horrid catch-everything blocks, showing exactly what block it is, and what is it catching and hiding. Run the test suite over the code thus instrumented, and you'll have a starting "blueprint" for the fixing job.

If you don't have a test suite, then, first, make one -- unit tests can wait (check out Feathers' great book on working with legacy code -- legacy code is most definitely your problem here;-), but you need a suite of integration tests that can be run automatically and tickle all the bugs you're supposed to fix.

As you go and fix bug after bug (many won't be caused by the excessively broad catch blocks, just hidden/"postponed" by them;-), be sure to work in a mostly "test-driven" manner: add a unit test that tickes the bug, confirm it breaks, fix the bug, rerun the unit test to confirm the bug's gone. Your growing unit test suite (with everything possible mocked out or faked) will run fast and you can keep rerunning cheaply as you work, to catch possible regressions ASAPs, when they're still easy to fix.

The kind of task you've been assigned is actually harder (and often more important) than "high prestige" SW development tasks such as prototypes and new architectures, but often misunderstood and under-appreciated (and therefore under-rewarded!) by management/clients; make sure to keep a very clear and open communication channel with the stakeholder, pointing out all the enormous amount of successful work you're doing, how challenging it is, and (for their sake more than your own), how much they would have saved by doing it right in the first place (maybe next time they will... I know, I'm a wild-eyed optimist by nature;-). Maybe they'll even assign you a partner on the task, and you can then do code reviews and pair programming, boosting productivity enormously.

And, last but not least, good luck!!! -- unfortunately, you'll need it... fortunately, as Jefferson said, "the harder I work, the more luck I seem to have";-)

Answer 7

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 8

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.

Answer 9

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));
}