Is this a bad practice to catch a non-specific exception such as System.Exception? Why?

I am currently doing a code review and the following code made me jump. I see multiple issues with this code. Do you agree with me? If so, how do I explain to my colleague that this is wrong (stubborn type...)?

• Catch a generic exception (Exception ex)
• The use of "if (ex is something)" instead of having another catch block
• We eat SoapException, HttpException and WebException. But if the Web Service failed, there not much to do.

Code:

try
{
    // Call to a WebService
}
catch (Exception ex)
{
    if (ex is SoapException || ex is HttpException || ex is WebException)
    {
        // Log Error and eat it.
    }
    else
    {
        throw;
    }
}

Solution 1:

The mantra is:

• You should only catch exceptions if you can properly handle them

Thus:

• You should not catch general exceptions.

In your case, yes, you should just catch those exceptions and do something helpful (probably not just eat them--you could throw after you log them).

Your coder is using throw (not throw ex) which is good.

This is how you can catch multiple, specific exceptions:

try
{
    // Call to a WebService
}
catch (SoapException ex)
{
    // Log Error and eat it
}
catch (HttpException ex)
{
    // Log Error and eat it
}
catch (WebException ex)
{
    // Log Error and eat it
}

This is pretty much equivalent to what your code does. Your dev probably did it that way to avoid duplicating the "log error and eat it" blocks.

Solution 2:

I am currently doing a code review and the following code made me jump. I see multiple issues with this code. Do you agree with me?

Not totally, see below.

• Catch a generic exception (Exception ex)

In general, catching a generic exception is actually ok as long as you rethrow it (with throw;) when you come to the conclusion that you can't handle it. The code does that, so no immediate problem here.

• The use of "if (ex is something)" instead of having another catch block

The net effect of the catch block is that only SoapException, HttpException, etc. are actually handled and all other exceptions are propagated up the call stack. I guess functionality-wise this is what the code is supposed to do, so there's no problem here either.

However, from a aesthetics & readability POV I would prefer multiple catch blocks to the "if (ex is SoapException || ..)". Once you refactor the common handling code into a method, the multiple catch blocks are only slightly more typing and are easier to read for most developers. Also, the final throw is easily overlooked.

• We eat SoapException, HttpException and WebException. But if the Web Service failed, there not much to do.

Here possibly lurks the biggest problem of the code, but it's hard to give advice without knowing more about the nature of the application. If the web service call is doing something that you depend on later then it's probably wrong to just log & eat the exceptions. Typically, you let the exception propagate to the caller (usually after wrapping it into e.g. a XyzWebServiceDownException), maybe even after retrying the webservice call a few times (to be more robust when there are spurious network issues).

Solution 3:

The problem with catching and re-throwing the same exception is that, although .NET does its best to keep the stack trace intact, it always ends up getting modified which can make tracking down where the exception actually came from more difficult (e.g. the exception line number will likely appear to be the line of the re-throw statement rather than the line where the exception was originally raised). There's a whole load more information about the difference between catch/rethrow, filtering, and not catching here.

When there is duplicate logic like this, what you really need is an exception filter so you only catch the exception types you're interested in. VB.NET has this functionality, but unfortunately C# doesn't. A hypothetical syntax might look like:

try
{
    // Call to a WebService
}
catch (Exception ex) if (ex is SoapException || ex is HttpException || /* etc. */)
{
    // Log Error and eat it
}

As you can't do this though, what I tend to do instead is use a lambda expression for the common code (you could use a delegate in C# 2.0), e.g.

Action<Exception> logAndEat = ex => 
    {  
        // Log Error and eat it
    };

try
{
    // Call to a WebService
}
catch (SoapException ex)
{
    logAndEat(ex);
}
catch (HttpException ex)
{
    logAndEat(ex);
}
catch (WebException ex)
{
    logAndEat(ex);
}