12

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?

Cœur
  • 37,241
  • 25
  • 195
  • 267
User
  • 3,244
  • 8
  • 27
  • 48
  • I wouldn't even catch any exception in this case. This would guarantee that the problem shows up immediately. Otherwise one is guessing around for the reason why this value is wrong and finds the problem only by scrolling through the log files. – mmmmmmmm Feb 24 '14 at 21:05

6 Answers6

8

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.

Community
  • 1
  • 1
Mikael Eliasson
  • 5,157
  • 23
  • 27
4

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;
    }
}
iheanyi
  • 3,107
  • 2
  • 23
  • 22
  • And it passes Code Analysis rule 1031 "Do not catch general exception types", if only you throw the rest of exception types forward. – Tomasz Przychodzki Feb 10 '16 at 11:41
  • 1
    Since C#6 it is ``catch (Exception ex) when (ex is FormatException || ex is ...)``. And don't use ``throw ex;``, always use ``throw;`` to keep the old exception 1:1. – ecth Mar 30 '16 at 13:41
2

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.

John Bledsoe
  • 17,142
  • 5
  • 42
  • 59
  • 6
    I disagree. That should not be your desired functionality. If something malicious is going on, then your code will just blissfully ignore it since it catches all exceptions as opposed to just the ones that it expects and knows how to handle. Saving yourself a minute of typing introduces security issues in your code and potentially masks bugs that you yourself have introduced. – iheanyi Mar 12 '14 at 21:08
  • 2
    I agree with @iheanyi. This is a bad practice. Exception handling is about *handling* exceptions. You should only catch the ones you know how to handle. Leave the others to bubble up. Keep in mind though, it's always a good idea to have a global exception handler that will catch all uncaught exceptions. This mechanism should at least log the error, but also have some intelligence to determine how to respond to the more serious exceptions. For example, what do you do if you get an IOException because your disk has run out of space? What if you were just swallowing that exception higher up? – mikesigs Apr 09 '14 at 15:37
  • 2
    I'm with @mikesigs on this. At the top level of your application, having a global catchall exception handler is appropriate. At this point, you still can't handle the exception and the appropriate response is either to log then "restart" your app or log then "terminate" your app. – iheanyi Apr 09 '14 at 15:49
  • 4
    @iheanyi: Do not use a catch-all at your upmost level but use System.AppDomain.UncatchedExceptionHandler. – mmmmmmmm May 13 '14 at 11:54
2

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.

supercat
  • 77,689
  • 9
  • 166
  • 211
1

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.

keithwarren7
  • 14,094
  • 8
  • 53
  • 74
  • 1
    Personally, I don't think it is silly to catch an exception and create a new one. What if, in the example I gave, I wanted to replace all the three exceptions thrown with a more generic `System.Configuration.ConfigurationErrorsException` with `ThisException` as an inner exception? – User Jun 30 '11 at 14:20
  • Clarification - I often see people catch an exception, create a new exception with the message 'error happened' and then not even provide the original exception as the inner to their new exception. Those are the boneheads of which I speak. – keithwarren7 Jun 30 '11 at 14:22
  • Oh... Ok then! Now I understand! – User Jun 30 '11 at 14:25
  • 2
    I agree with you that in general that behaviour is deeply irksome. However, sometimes it is warranted. For example, when catching an exception at a "trust boundary" this is the right thing to do. An attacker outside the trust boundary could be attempting to determine secret implementation details of your system by feeding it bad data and examining the detailed error information that comes back. In that scenario, getting rid of all the detailed information and replacing it with "something failed, we'll log the results" is desirable. – Eric Lippert Jun 30 '11 at 18:03
  • I would think it was bad practice in general to propagate details that come from a raw exception up into the user space. Well written, a program should log such information but hide it from anyone not trusted. If you are talking about something outside of web scope, something local machine and you are worried about details being sniffed I think the battle is already lost because without physical security there is no security IMHO. – keithwarren7 Jun 30 '11 at 18:30
1

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 :)

Blindy
  • 65,249
  • 10
  • 91
  • 131