9

I'm running some code through FxCop and am currently looking at clearing all of the non-breaking violations.

The code itself has a few instances of try/catch blocks which just catch general exceptions;

try { // Some code in here that could throw an exception } catch(Exception ex) { // Exception Thrown ... sort it out! }

Now we all know this is is bad practice but I thought I knew how to do it properly - but FxCop has other ideas!

Suppose that the code in the try block could throw an IO Exception - and only an IO exception. There should be nothing wrong with doing this:

try { // Code in here that can only throw an IOException } catch (System.IO.IOException ioExp) { // Handle the IO exception or throw it } catch (System.Exception ex) { // Catch otherwise unhandled exception }

But FxCop disagrees with me ... it still flags this as a violation because I'm catching System.Exception.

Is this really bad practice or should/can I safely ignore this violation?

Improve this question
4
  • 1
    It depends... what are you doing in "// Catch otherwise unhandled exception"? Commented Oct 27, 2009 at 10:15
  • The application is capable of sending emails to our Helpdesk - which get passed to Development - so we'd probably trigger one of these with the stack trace in it. Commented Oct 27, 2009 at 10:31
  • Are you absolutely sure that the code block cannot throw a OutOfMemoryException for example, or another fatal error..? Commented Oct 27, 2009 at 15:10
  • The above was hypothetical - the method calls in the actual code could throw one of 8 different exceptions. However, can you ever be sure that they are the only ones possible? Hence the question ;-) Commented Oct 27, 2009 at 15:41

8 Answers 8

Reset to default
7

I agree with FxCop and most of the other answers here: don't catch System.Exception ever, unless it is at the highest level in your application to log unexpected (and thus fatal) exceptions and then bomb the application. Also check out this question that's basically about the same thing.

Some other valid exceptions might be:

  • To catch exceptions for the sole reason of rethrowing a more descriptive one.
  • In unit test code that needs something more complex than an ExpectedExceptionAttribute.
  • In facade library code that only exists to shield callers from intricacies of calling some external (web) service, while never actually failing itself, no matter how badly the external system might go down.
Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

What if you want to log with the exception certain values of the parameters of the lower level method it occurs? Can't do that from only the "highest level in your application".
@Swanny: What about Exception.Data for this in the general case? Or custom exception types with extra properties? Not a problem. And you can also catch an exception at the lower level where you have access to that data, log it if needed, and package the exception up into a more descriptive (custom) exception type that is throw. Not a problem, therefore, right?
You've still got to populate the Data collection, and so you have to catch the exception to do so, and so we have yet another exception to don't ever. And here is another one. Suppose I'm processing a large batch file of independent records and I get an unexpected exception processing a record. Do I throw this unexpected exception up to "the highest level"? Nope, I log it and move onto the next record, probably till I get to the end of file or the max bad records allowed, whichever comes first.
Note that if you're just logging and rethrowing, that's OK. The point is that your code is unlikely to know how to handle a non-specific exception.
5

I agree with the other answers that catch (Exception ex) is fine if you either

  1. just log and rethrow the exception or
  2. do it at the highest level (UI) of your application.

There is one more situation that comes to my mind where it could make sense: When writing unattended code (e.g. some system service), it can make perfect sense to keep the program running if some (well-defined) sub-task fails for whatever reason. Of course, care must be taken to ensure that the reason for the problem gets logged and (maybe) the task is tried again after some time.

Improve this answer

Comments

4

Grey area...

In an ideal world you'd alway catch an explicit exception because, in theory, those are the only kind you can sensibly deal with - anything else should percolate through to some top level handler.

The trouble is we don't necessarily live in an ideal world and may well want to use a generic handler to accumulate additional information about the generic exception (parameters etc) into the exception and/or to perform additional logging before passing the exception back up the tree and in any case - at the appropriate level - to so arrange things that our end users don't see raw exceptions in the UI. The counter to this would be to suggest that when new errors occur at a low level (rather than arriving at your app level handler) you then add exception specific handling that can do your capture for you - but there can be deployment issues with this as a solution where including a generic case in bits of code that are, for whatever reason, a bit more prone to unhandled exceptions that one might like.

If it were me I'd probably be considering the flag on an assembly by assembly basis...

p.s. I'm assuming that at no point to you have a handler that just swallows the exception and allows the app to carry on.

Improve this answer

3 Comments

Hang on. You only catch the exceptions you expect. So unexpected exceptions are left to crash your program? That's not what I would call a professional solution. Look at the WinForms general exception handling that was introduced for WinForms (1.1 or 2.0). And if I needed to process 1 million independant records in a file, I'd rather catch the unexpected exception, record the error against a log along with the record number then move onto the next record, possibly with a max allowed bad record number.
Erm, I most emphatically did not suggest that unexpected exceptions should crash your program. c.f. "top level handler" - the specific case you raise is also a very good point hence the further observation that you need to deal with the notion on a case by case basis.
Closest answer, I think, to the spirit of the language. I have read interviews with Anders where the question of checked vs unchecked has been raised and his response is that if developers were going to be handling errors at the top level anyway the language shouldn't interfere. It's very much a case of pragmatism over purity.
3

If you writing library(framework) then FxCop is 100% right.

You catch exceptions - what it's mean? that you know why they throwed. Are you sure you know all possible reasons?

If you writing just application then variations possible. For example if you catch all unhandled exceptions for logging.

Improve this answer

6 Comments

I'm sure you can never know all of the reasons but looking at the method calls within the try block I know that the current piece of code could throw at least 8 different exception types.
than you must handle them all separately! One Exception may be because invalid file name, another because disk IO error. You should respond separately on this since it will lead communication with your users. or just ignore FxCop if it seems boring for you :-).
But if I'm just going to 'return false;' from each catch block it seems pretty pointless. We are only just starting with Code Analysis tools like FxCop and need to review our usage of them.
Yeah, like before we had TryParse, the only easy way to tell if a string was an int (easy codewise that is) was to call parse, and return true or catch the exception and return false. The only real issue here is that catching an exception for a normal case can be a little slow, and if it happens in a loop intensive way this can cause your app to slow.
@Swanny thats why TryParse was added because 1)It'S not a good design choice when you catch an exception just to check if your data in right format or not 2) Exceptions are slow
|
3

You are supposed to swallow exceptions you know how to handle. Catching the exception and not throwing it (either directly or wrapped in a specific exception) means you know the specifics of the exception in the context of the application. Since Exception can be anything, it's unlikely you know how to deal with it.

If you're just logging the exception it's no big deal, just log it and throw it for a outer layer to handle or catch.

Improve this answer

3 Comments

Except of course, what does the outer layer do with the exception if it didn't expect it?
Usually, it won't do much with it, but a last-chance global exception handler can catch it, provide feedback to the user and prevent the application to close. It's not the burden of a class library to do this, it's the host responsability.
Which sounds like a perfectly sound way of handling an unexpected exception. But would you accept there are cases where a class library wouldn't throw the unexpected exception, like the batch file of independent records I've mentioned more than once already on this page.
2

What does FxCopy say the specific violation is? If you want to just log information about an exception then rethrow it then this is perfectly valid, though there are other ways this might be done. If you are re-raising it though, make sure you just use:

 throw;

and not 

 throw ex;

Maybe that's the issue?

Improve this answer

4 Comments

There are a few instances of this but that is flagged as a RethrowToPreserveStackDetails violation.
Oh good. Go FxCop then. What was the name of the violation you were getting for capturing System.Exception?
The violation in question is DoNotCatchGeneralExceptionTypes : msdn2.microsoft.com/library/ms182137(VS.90).aspx
Ok, got it now. They don't want you to hide details of a run time exception. Sounds like someone got a little prostate fixated here. Consider the point and if doesn't apply, plow on. I've caught System.Exception plenty of times for perfectly valid reasons, and there are plenty of clear cases where you would need to.
2

If the code in the block should only throw an IOException, then why do you want to catch System.Exception as well?

Then, the degree of conformancy to FxCop guidelines you want to achieve is a choice of yours. I would see nothing bad in catching every exception at the highest possible level and log all the information you can, for example.

Improve this answer

Comments

2

You should only catch exceptions that you can do something about. Otherwise let the exception propagate up to the caller or if, for instance, in a Gui app the exceptions should be handled by a generic catch all handler and then logged. See: UnhandledException So don't catch unless you intended to do something with it.. admittedly that could simply be logging the error.

Edit Also look at Application.ThreadException

Improve this answer

5 Comments

True - but surely unless you catch ALL exceptions at some point it's gonna reach the UI as an Unhandled Exception.
@DilbertDave: Which as far as I'm concerned should be the only exception to the rule of never catching System.Exception.
@DilbertDave isn't that why you assign a handler to the UnhandledException event?
Actually WinForm global exception handling was improved so that you could catch all exception on the GUI thread separate from the other threads. For most apps this was the same thing. I typically have a common dialog that says something along the lines of "I'm sorry the last operation could not be completed because of the following unexpected error:" + ex.Message. One button emails the details to the service desk. Another closes the dialog, giving the user a chance to correct the error and try again, or at least copy the details they have entered before closing. Rather than just crashing.
I should probably point out that this is an ASP.NET application

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.