Is This a Bad Practice to Catch a Non-Specific Exception Such as System.Exception? Why

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

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.

Is it really that bad to catch a general exception?

Obviously this is one of those questions where the only real answer is "it depends."

The main thing it depends on is where your are catching the exception. In general libraries should be more conservative with catching exceptions whereas at the top level of your program (e.g. in your main method or in the top of the action method in a controller, etc) you can be more liberal with what you catch.

The reason for this is that e.g. you don't want to catch all exceptions in a library because you may mask problems that have nothing to do with your library, like "OutOfMemoryException" which you really would prefer bubbles up so that the user can be notified, etc. On the other hand, if you are talking about catching exceptions inside your main() method which catches the exception, displays it and then exits... well, it's probably safe to catch just about any exception here.

The most important rule about catching all exceptions is that you should never just swallow all exceptions silently... e.g. something like this in Java:

try { 
    something(); 
} catch (Exception ex) {}

or this in Python:

try:
    something()
except:
    pass

Because these can be some of the hardest issues to track down.

A good rule of thumb is that you should only catch exceptions that you can properly deal with yourself. If you cannot handle the exception completely then you should let it bubble up to someone who can.

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

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.

Why not catch general Exceptions

Swallowing exceptions is a dangerous practice because:

It can cause the user to think something succeeded when it actually failed.
It can put your application into states that you didn't plan for.
It complicates debugging, since it's much harder to find out where the failure happened when you're dealing with bizarre/broken behavior instead of a stack trace.

As you can probably imagine, some of these outcomes can be extremely catastrophic, so doing this right is an important habbit.

Best Practice

First off, code defensively so that exceptions don't occur any more than necessary. They're computationally expensive.

Handle the expected exceptions at a granular level (for example: FileNotFoundException) when possible.

For unexpected exceptions, you can do one of two things:

Let them bubble up normally and cause a crash
Catch them and fail gracefully

Fail Gracefully?

Let's say you're working in ASP.Net and you don't want to show the yellow screen of death to your users, but you also don't want problems to be hidden from the dev team.

In our applications, we usually catch unhandled exceptions in global.asax and then do logging and send out notification emails. We also show a more friendly error page, which can be configured in web.config using the customErrors tag.

That's our last line of defense, and if we end up getting an email we jump on it right away.

That type of pattern is not the same as just swallowing exceptions, where you have an empty Catch block that only exists to "pretend" that the exception did not occur.

Other Notes

In VS2010, there's something called intellitrace coming that will allow you to actually email the application state back home and step through code, examine variable values at the time of the exception, and so on. That's going to be extremely useful.

Why is the Catch(Exception) almost always a bad Idea?

Because when you catch exception you're supposed to handle it properly. And you cannot expect to handle all kind of exceptions in your code. Also when you catch all exceptions, you may get an exception that cannot deal with and prevent code that is upper in the stack to handle it properly.

The general principal is to catch the most specific type you can.

Main method code entirely inside try/catch: Is it bad practice?

Wrapping any piece of code in a try/catch block without a good reason is bad practice.

In the .NET programming model, exceptions should be reserved for truly exceptional cases or conditions. You should only try to catch exceptions that you can actually do something about. Furthermore, you should should hardly ever catch the base System.Exception class (but rather prefer to catch the more specific, derived exception classes you can handle). And should a truly unexpected exception be encountered during the course of your program's execution, you actually should crash.

Obviously the "correct" answer would have to be made on a case-by-case basis, depending on what's going on inside that // code placeholder in your catch block. But if you're asking for a general rule or "best practice", you should always have a specific reason to catch exceptions, not just wrap all of your code in a giant try/catch block as a matter of course without thinking about it.

Note that if you're simply trying to catch any unhandled exceptions that might occur for the purposes of logging or error reporting, you should be using the AppDomain.UnhandledException event. This is a notification-only event, so it doesn't allow you to handle those exceptions, but it is the right place to implement your logging or error reporting functionality after your application has crashed.

EDIT: As I was catching up on my reading of Raymond Chen's excellent blog, "The Old New Thing", I noticed that he had recently published an article on a similar topic. It's specific to COM, rather than the .NET Framework, but the general concepts regarding error handling are equally applicable to both environments. I thought I'd share a couple of gems from the article here, in support of my [apparently quite controversial] opinion.

Historically, COM placed a giant try/except around your server's methods. If your server encountered what would normally be an unhandled exception, the giant try/except would catch it and turn it into the error RPC_E_SERVERFAULT. It then marked the exception as handled, so that the server remained running, thereby "improving robustness by keeping the server running even when it encountered a problem."

Mind you, this was actually a disservice.

The fact that an unhandled exception occurred means that the server was in an unexpected state. By catching the exception and saying, "Don't worry, it's all good," you end up leaving a corrupted server running.

[ . . . ]

Catching all exceptions and letting the process continue running assumes that a server can recover from an unexpected failure. But this is absurd. You already know that the server is unrecoverably toast: It crashed!

Much better is to let the server crash so that the crash dump can be captured at the point of the failure. Now you have a fighting chance of figuring out what's going on.

You can [and should] read the whole article here on his blog: How to turn off the exception handler that COM "helpfully" wraps around your server.

Why is except: pass a bad programming practice?

As you correctly guessed, there are two sides to it: Catching any error by specifying no exception type after except, and simply passing it without taking any action.

My explanation is “a bit” longer—so tl;dr it breaks down to this:

Don’t catch any error. Always specify which exceptions you are prepared to recover from and only catch those.
Try to avoid passing in except blocks. Unless explicitly desired, this is usually not a good sign.

But let’s go into detail:

Don’t catch any error

When using a try block, you usually do this because you know that there is a chance of an exception being thrown. As such, you also already have an approximate idea of what can break and what exception can be thrown. In such cases, you catch an exception because you can positively recover from it. That means that you are prepared for the exception and have some alternative plan which you will follow in case of that exception.

For example, when you ask for the user to input a number, you can convert the input using int() which might raise a ValueError. You can easily recover that by simply asking the user to try it again, so catching the ValueError and prompting the user again would be an appropriate plan. A different example would be if you want to read some configuration from a file, and that file happens to not exist. Because it is a configuration file, you might have some default configuration as a fallback, so the file is not exactly necessary. So catching a FileNotFoundError and simply applying the default configuration would be a good plan here. Now in both these cases, we have a very specific exception we expect and have an equally specific plan to recover from it. As such, in each case, we explicitly only except that certain exception.

However, if we were to catch everything, then—in addition to those exceptions we are prepared to recover from—there is also a chance that we get exceptions that we didn’t expect, and which we indeed cannot recover from; or shouldn’t recover from.

Let’s take the configuration file example from above. In case of a missing file, we just applied our default configuration and might decide at a later point to automatically save the configuration (so next time, the file exists). Now imagine we get a IsADirectoryError, or a PermissionError instead. In such cases, we probably do not want to continue; we could still apply our default configuration, but we later won’t be able to save the file. And it’s likely that the user meant to have a custom configuration too, so using the default values is likely not desired. So we would want to tell the user about it immediately, and probably abort the program execution too. But that’s not something we want to do somewhere deep within some small code part; this is something of application-level importance, so it should be handled at the top—so let the exception bubble up.

Another simple example is also mentioned in the Python 2 idioms document. Here, a simple typo exists in the code which causes it to break. Because we are catching every exception, we also catch NameErrors and SyntaxErrors. Both are mistakes that happen to us all while programming and both are mistakes we absolutely don’t want to include when shipping the code. But because we also caught those, we won’t even know that they occurred there and lose any help to debug it correctly.

But there are also more dangerous exceptions which we are unlikely prepared for. For example, SystemError is usually something that happens rarely and which we cannot really plan for; it means there is something more complicated going on, something that likely prevents us from continuing the current task.

In any case, it’s very unlikely that you are prepared for everything in a small-scale part of the code, so that’s really where you should only catch those exceptions you are prepared for. Some people suggest to at least catch Exception as it won’t include things like SystemExit and KeyboardInterrupt which by design are to terminate your application, but I would argue that this is still far too unspecific. There is only one place where I personally accept catching Exception or just any exception, and that is in a single global application-level exception handler which has the single purpose to log any exception we were not prepared for. That way, we can still retain as much information about unexpected exceptions, which we then can use to extend our code to handle those explicitly (if we can recover from them) or—in case of a bug—to create test cases to make sure it won’t happen again. But of course, that only works if we only ever caught those exceptions we were already expecting, so the ones we didn’t expect will naturally bubble up.

Try to avoid passing in except blocks

When explicitly catching a small selection of specific exceptions, there are many situations in which we will be fine by simply doing nothing. In such cases, just having except SomeSpecificException: pass is just fine. Most of the time though, this is not the case as we likely need some code related to the recovery process (as mentioned above). This can be for example something that retries the action again, or to set up a default value instead.

If that’s not the case though, for example, because our code is already structured to repeat until it succeeds, then just passing is good enough. Taking our example from above, we might want to ask the user to enter a number. Because we know that users like to not do what we ask them for, we might just put it into a loop in the first place, so it could look like this:

def askForNumber ():
    while True:
        try:
            return int(input('Please enter a number: '))
        except ValueError:
            pass

Because we keep trying until no exception is thrown, we don’t need to do anything special in the except block, so this is fine. But of course, one might argue that we at least want to show the user some error message to tell him why he has to repeat the input.

In many other cases though, just passing in an except is a sign that we weren’t really prepared for the exception we are catching. Unless those exceptions are simple (like ValueError or TypeError), and the reason why we can pass is obvious, try to avoid just passing. If there’s really nothing to do (and you are absolutely sure about it), then consider adding a comment why that’s the case; otherwise, expand the except block to actually include some recovery code.

`except: pass`

The worst offender though is the combination of both. This means that we are willingly catching any error although we are absolutely not prepared for it and we also don’t do anything about it. You at least want to log the error and also likely reraise it to still terminate the application (it’s unlikely you can continue like normal after a MemoryError). Just passing though will not only keep the application somewhat alive (depending on where you catch of course), but also throw away all the information, making it impossible to discover the error—which is especially true if you are not the one discovering it.

So the bottom line is: Catch only exceptions you really expect and are prepared to recover from; all others are likely either mistakes you should fix or something you are not prepared for anyway. Passing specific exceptions are fine if you really don’t need to do something about them. In all other cases, it’s just a sign of presumption and being lazy. And you definitely want to fix that.

Which exceptions shouldn't I catch?

You don't need a list of 'bad' exceptions, you should treat everything as bad by default. Only catch what you can handle and recover from. CLR can notify you of unhandled exceptions so that you can log them appropriately. Swallowing everything but a black listed exceptions is not a proper way to fix your bugs. That would just mask them. Read this and this.

Do not exclude any special exceptions when catching for the purpose
of transferring exceptions.

Instead of creating lists of special exceptions in your catch clauses,
you should catch only those exceptions that you can legitimately
handle. Exceptions that you cannot handle should not be treated as
special cases in non-specific exception handlers. The
following code example demonstrates incorrectly testing for special
exceptions for the purposes of re-throwing them.

public class BadExceptionHandlingExample2 {
    public void DoWork() {
        // Do some work that might throw exceptions.
    }
    public void MethodWithBadHandler() {
        try {
            DoWork();
        } catch (Exception e) {
            if (e is StackOverflowException ||
                e is OutOfMemoryException)
                throw;
            // Handle the exception and
            // continue executing.
        }
    }
}

Few other rules:

Avoid handling errors by catching non-specific exceptions, such as
System.Exception, System.SystemException, and so on, in application
code. There are cases when handling errors in applications is
acceptable, but such cases are rare.

An application should not handle exceptions that can result in an
unexpected or exploitable state. If you cannot predict all possible
causes of an exception and ensure that malicious code cannot exploit
the resulting application state, you should allow the application to
terminate instead of handling the exception.

Consider catching specific exceptions when you understand why it
will be thrown in a given context.

You should catch only those exceptions that you can recover from. For
example, a FileNotFoundException that results from an attempt to open
a non-existent file can be handled by an application because it can
communicate the problem to the user and allow the user to specify a
different file name or create the file. A request to open a file that
generates an ExecutionEngineException should not be handled because
the underlying cause of the exception cannot be known with any degree
of certainty, and the application cannot ensure that it is safe to
continue executing.

Eric Lippert classifies all exceptions into 4 groups: Fatal, 'Boneheaded', Vexing, Exogenous. Following is my interpretation of Eric's advice:

  Exc. type | What to do                          | Example
------------|-------------------------------------|-------------------
Fatal       | nothing, let CLR handle it          | OutOfMemoryException
------------|-------------------------------------|-------------------
Boneheaded  | fix the bug that caused exception   | ArgumentNullException
------------|-------------------------------------|-------------------
Vexing      | fix the bug that caused exception   | FormatException from 
            | (by catching exception  because     | Guid constructor
            | the framework provides no other way | (fixed in .NET 4.0 
            | way of handling). Open MS Connect   | by Guid.TryParse)
            | issue.                              | 
------------|-------------------------------------|-------------------
Exogenous   | handle exception programmatically   | FileNotFoundException

This is roughly equivalent to Microsoft's categorization: Usage, Program error and System failure.
You can also use static analysis tools like FxCop to enforce some of these rules.

Best practice for Java exception handling

First, unless you have very good reason, never catch RuntimeException, Exception or Throwable. These will catch most anything that is thrown, and Throwable will catch everything, even those things you're not meant to catch, like OutOfMemoryError.

Second, avoid catching runtime exceptions unless it directly impedes with the critical operation of your program. (But seriously, if anyone sees you catch a NullPointerException, they are within their full rights to call you out on it.) The only exceptions you should be bothering with are those that you are required to handle. Out of your list of exceptions, the only one you should bother with is IOException. The rest are the result of not enough tests, or sloppy coding; those shouldn't occur in the normal run time of your application.

Third, in Java 7, you have the ability to do a multi-catch statement for your exceptions, if the exceptions are mutually exclusive. The example linked does a good job of explaining it, but if you were to encounter code that threw both an IOException and an SQLException, you could handle it like this:

try {
    // Dodgy database code here
catch (IOException|SQLException ex) {
    logger.log(ex);
    throw ex;
}

This cleans things up a bit, as you don't have unwieldy and huge chains of exceptions to catch.

Should I catch and wrap general Exception?

Short answer: Don't do this unless there is some reason that you must. Instead, catch specific exceptions you can deal with at the point you can deal with them, and allow all other exceptions to bubble up the stack.

TL;DR answer: It depends what you're writing, what is going to be calling your code, and why you feel that you need to introduce a custom exception type.

The most important question to ask, I think, is if I catch, what benefit does my caller get?

Do not hide information that your caller needs
Do not require your caller to jump through any more hoops than necessary

Imagine you're processing a low-level data source; maybe you're doing some encoding or deserialisation. Our system is nice and modular, so you don't have much information about what your low-level data source is, or how your code is being called. Maybe your library is deployed on a server listening to a message queue and writing data to disk. Maybe it's on a desktop and the data is coming from the network and being displayed on a screen.

There are lots of varied exceptions that might occur (DbException, IOException, RemoteException, etc), and you have no useful means of handling them.

In this situation, in C#, the generally accepted thing to do is let the exception bubble up. Your caller knows what to do: the desktop client can alert the user to check their network connection, the service can write to a log and allow new messages to queue up.

If you wrap the exception in your own MyAwesomeLibraryException, you've made your caller's job harder. Your caller now needs to:-

Read and understand your documentation
Introduce a dependency on your assembly at the point they want to catch
Write extra code to interrogate your custom exception
Rethrow exceptions they don't care about.

Make sure that extra effort is worth their time. Don't do it for no reason!

If the main justification for your custom exception type is so that you can provide a user with friendly error messages, you should be wary of being overly-nonspecific in the exceptions you catch. I've lost count of the number of times - both as a user and as an engineer - that an overzealous catch statement has hidden the true cause of an issue:-

try
{
  GetDataFromNetwork("htt[://www.foo.com"); // FormatException?

  GetDataFromNetwork(uriArray[0]); // ArrayIndexOutOfBounds?

  GetDataFromNetwork(null); // ArgumentNull?
}
catch(Exception e)
{
  throw new WeClearlyKnowBetterException(
    "Hey, there's something wrong with your network!", e);
}

or, another example:-

try
{
  ImportDataFromDisk("C:\ThisFileDoesNotExist.bar"); // FileNotFound?

  ImportDataFromDisk("C:\BobsPrivateFiles\Foo.bar"); // UnauthorizedAccess?

  ImportDataFromDisk("C:\NotInYourFormat.baz"); // InvalidOperation?

  ImportDataFromDisk("C:\EncryptedWithWrongKey.bar"); // CryptographicException?
}
catch(Exception e)
{
  throw new NotHelpfulException(
    "Couldn't load data!", e); // So how do I *fix* it?
}

Now our caller has to unwrap our custom exception in order to tell the user what's actually gone wrong. In these cases, we've asked our caller to do extra work for no benefit. It is not intrinsically a good idea to introduce a custom exception type wrapping all exceptions. In general, I:-

Catch the most specific exception I can
At the point I can do something about it
Otherwise, I just let the exception bubble up
Bear in mind that hiding the details of what went wrong isn't often useful

That doesn't mean you should never do this!

Sometimes Exception is the most specific exception you can catch because you want to handle all exceptions in the same way - e.g. Log(e); Environment.FailFast();
Sometimes you have the context to handle an exception right at the point where it gets thrown - e.g. you just tried to connect to a network resource and you want to retry
Sometimes the nature of your caller means that you cannot allow an exception to bubble up - e.g. you're writing logging code and you don't want a logging failure to "replace" the original exception that you're trying to log!
Sometimes there's useful extra information you can give your caller at the point an exception is thrown that won't be available higher up the call stack - e.g. in my second example above we could catch (InvalidOperationException e) and include the path of the file we were working with.
Occasionally what went wrong isn't as important as where it went wrong. It might be useful to distinguish a FooModuleException from a BarModuleException irrespective of what the actual problem is - e.g. if there's some async going on that might otherwise prevent you usefully interrogating a stack trace.
Although this is a C# question, it's worth noting that in some other languages (particularly Java) you might be forced to wrap because checked exceptions form part of a method's contract - e.g. if you're implementing an interface, and the interface doesn't specify that the method can throw IOException.

This is all pretty general stuff, though. More specifically to your situation: why do you feel you need the custom exception type? If we know that, we can maybe give you some better tailored advice.

Is This a Bad Practice to Catch a Non-Specific Exception Such as System.Exception? Why