CODEWITHSUNDEEP
c#exception
User
User

Reputation: 3274

Is it always a bad practice to catch System.Exception?

Please consider the following piece of code, which throws three different exceptions (namely, System.Configuration.ConfigurationErrorsException, System.FormatException and System.OverflowException):

int SomeInt = Convert.ToInt32(ConfigurationManager.AppSettings["SomeIntValue"]);

The exceptions are different, and so in practice I should have three different catch blocks to handle each particular exception. However, in this particular case, all exceptions are handled the same way: a log is written to, say, EventViewer, and a message informing of a configuration error is displayed... In this particular cause, is it too bad to use

try
{
    int SomeInt = ConfigurationManager.AppSettings["SomeIntValue"];
}
catch (Exception ThisException)
{
    /* Log and display error message. */
}

or should I, instead, use the three catch blocks and repeat the code within each of them?

Upvotes: 12

Views: 9434

Answers (6)

iheanyi
iheanyi

Reputation: 3113

It is bad practice to catch System.Exception . . . or better yet, it is bad practice to handle System.Exception anywhere but the top level of your application. What you should to is:

  1. Catch System.Exception
  2. Test the exception for the types you plan to handle identically
  3. Rethrow if it's not one of those.

Example code:

catch (Exception ex)
{
    if (ex is FormatException || ex is OverflowException || ex is ConfigurationErrorsException) {
        CommonHandler();
    }
    else {
        throw;
    }
}

Upvotes: 4

supercat
supercat

Reputation: 81115

What is really needed, but the .net exception hierarchy doesn't provide, is a clean way of distinguishing exceptions which mean "The requested operation didn't happen, but the system state is essentially fine except to the extent implied by the operation not having happened" from those which mean "The CPU is on fire, and even trying to save the current user's work would likely as not make things worse." There are a lot of contexts in which one really should endeavor to catch all exceptions of the first type, while ideally not catching those of the second. While there a few gradations beyond the two above, generally when catching exceptions one doesn't really care about the distinction between an InvalidArgumentException or an InvalidOperationException; what one cares about is whether the overall system state is valid or corrupted.

As it is, if one is making a call to e.g. a file-import plug-in and it throws an exception, I'm not really sure one can do except try to catch and rethrow really bad exceptions, while having all other exceptions put up a "This file could not be opened" dialog box. Hopefully the state of the system at that point is essentially as it would be had the user not tried to open the file, but without some standardized way of indicating exception severity, I don't think there's any way to be sure.

Incidentally, if I had my druthers, there would be a class ExceptionBase, from which all exceptions would derive; most exceptions would derive from Exception (which would in turn derive from ExceptionBase) but things like ThreadAbortException, StackOverflowException, OutOfMemoryException, etc. would be derived from CriticalException. That way one could catch most 'unexpected' exceptions without accidentally stifling the really bad ones.

Upvotes: 2

Blindy
Blindy

Reputation: 67352

In this particular case, it is entirely possible to use different catch blocks, as you do have different actions you can take depending on what exception you get.

For the first two you can set a reasonable default silently as to not annoy the user needlessly, then hope the problem fixes itself when you save your file, and for the latter exception you can ask the user if he wants to continue with the loading process (in which case you set a reasonable default), since the configuration file is obviously corrupted.

In general, the thinking is this: Do I need to take different actions in regards to different kinds of exceptions being thrown? More often than not, the answer is no, so a blank catch(Exception ex){ /* log... */ } (or even no catch at all if the exception is invariably fatal) is enough.

Edit: Blank as in blank check, not empty block :)

Upvotes: 1

keithwarren7
keithwarren7

Reputation: 14280

It is not necessarily bad practice, the bad practice usually occurs when you do silly things in the catch block. Catching the base exception class is a good safety measure assuming you need to do something when the error occurs. The case for catching a specific exception class is best manifest when that specific class gives you information you can act on, catching SqlException for example can help you look at certain properties specific to that class and let you act on the problem.

Often times people will catch an exception and do something silly like create a new exception or even worse, swallow the exception details.

An often missed pattern is that you can catch, act an rethrow.

catch(Exception ex)
{
 //do something
 throw;
}

thus preserving the exception details.

Upvotes: 1

Mikael Eliasson
Mikael Eliasson

Reputation: 5227

See C# Exception Handling Fall Through for a discussion around this problem.

In short, if you catch the general Exception you should check if it is one of the expected types and if not rethrow it.

Update(and a bit out of scope)

Also there are a few times I think it's valid to do a catch all. This is very rare but sometimes in webservices or if you do something on the background thread in asp.net as and exception there will restart the whole application and people might lose their session if you have taken a dependency on that.

Upvotes: 8

John Bledsoe
John Bledsoe

Reputation: 17642

I don't think it's bad practice. If your desired functionality is "whenever this code throws an exception, then take these actions" then I think catching System.Exception is perfectly appropriate.

The fact that you're wrapping a very specific framework function instead of a large block of custom code helps in my opinion as well.

Upvotes: 2

Related Questions