What is the least disruptive way to remove over-zealous try..catch blocks?

Question

A code base I have inherited is riddled with the error-hiding anti-pattern:

public void foo() {
  try {
    //Entire body of method, sometimes only 5 lines but often 500+
  } catch (Exception e) {
    Logger.LogToFile(e.msg); 
  }
}

The number of instances of this pattern are in the order of hundreds.

My approach was to just remove the try..catch block entirely, given we already have a single exception-handler at the top level. This was unpopular as it was felt I was changing behaviour. (More exceptions would bubble up).

Ideally, I would go through each of the 100s of instances of 10-500 lines and find what exceptions could possibly be thrown. Then I'd only squash those exceptions in tighter blocks:

public void foo() {
  //...
  try {
    // ~1-5 lines
  } catch (ArgumentInvalidException e) {
    Logger.LogToFile(e.ToString()); 
    //TODO handle
  }
  //...
  try {
    // ~1-5 lines
  } catch (UnitConversionException e) {
    Logger.LogToFile(e.ToString()); 
    //TODO handle
  }
  //...
}

This is an intimidating amount of effort.

Can anyone think of a clever approach to fixing this?

Related - Removing excessive try-catch blocks

Instead of putting try-catches everywhere to log stuff, take a look at Aspect Oriented Programming. [PostSharp](http://www.postsharp.net/) seems the best tool to do that job. I think AOP is the best thing for your horizontal concerns. — Pierre-Luc Pineault, Jul 12 '13 at 16:38
If you've got 500+ lines of code in a method, you've got a whole other set of problems. I think I'd try to tackle those problems before I worried about exception hiding. Whittle away at those outrageously long methods - just extract methods at every opportunity, for starters - and things will improve. And it will be easier to address the exception hiding. — Carl Manaster, Jul 12 '13 at 16:52
_This was unpopular as it was felt I was changing behaviour_ ... so what exactly is they are asking you to do with these catch blocks if it's not changing the behaviour? — muratgu, Jul 12 '13 at 17:02
Huurrrghgh... This instantly reminds me of a dreaded 'ON ERROR RESUME NEXT' drama. With a little defensive programming, one should never have to catch an expected exception. Exceptions are perfect for situations that 'break the logical flow of events', e.g. aborting a method in the middle of something and bailing out of any uncorrectable situation (network disappeared suddenly). Exceptions should not be used to handle parsing errors. C# has excellent `TryParse` methods for that. Use exceptions for guarding input, when Assertions cannot be used properly. But i am digressing... — Rob van der Veer, Jul 13 '13 at 19:34

score 1 · Answer 1 · answered Jul 13 '13 at 18:35

Maybe "Extract Method" http://refactoring.com/catalog/extractMethod.html will help in this case. When I write a code, I try to distinguish between errors and exceptions. Errors != Exceptions in common case. Exceptions are too expensive, and should occur only in exceptional situations.

I will explain:

try
{
    int.Parse("..")
}
catch()
{
    // parsing Exception
}

vs

if(int.TryParse("..", out value)
{
    // parsing Error
}

What is the least disruptive way to remove over-zealous try..catch blocks?

1 Answers1