Should I catch and wrap general Exception?

Question

Can following code be considered as a good practice? If not, why?

try
{
    // code that can cause various exceptions...
}
catch (Exception e)
{
    throw new MyCustomException("Custom error message", e);
}

Answer 1

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

1. Catch the most specific exception I can

2. At the point I can do something about it

3. Otherwise, I just let the exception bubble up

4. Bear in mind that hiding the details of what went wrong isn't often useful

That doesn't mean you should never do this!

1. 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();

2. 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

3. 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!

4. 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.

5. 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.

6. 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.

Answer 2

There's an excellent blog post by Eric Lippert, "Vexing exceptions". Eric answers your question with some universal guidelines. Here is a quote from the "sum up" part:

• Don’t catch fatal exceptions; nothing you can do about them anyway, and trying to generally makes it worse.
• Fix your code so that it never triggers a boneheaded exception – an "index out of range" exception should never happen in production code.
• Avoid vexing exceptions whenever possible by calling the “Try” versions of those vexing methods that throw in non-exceptional circumstances. If you cannot avoid calling a vexing method, catch its vexing exceptions.
• Always handle exceptions that indicate unexpected exogenous conditions; generally it is not worthwhile or practical to anticipate every possible failure. Just try the operation and be prepared to handle the exception.

Answer 3

No, generally, you should not do that: this may mask the real exception, which may indicate programming issues in your code. For example, if the code inside the try / catch has a bad line that causes an array index out of bound error, your code would catch that, too, and throw a custom exception for it. The custom exception is now meaningless, because it reports a coding issue, so nobody catching it outside your code would be able to do anything meaningful with it.

On the other hand, if the code inside the try / catch throws exceptions that you expect, catching and wrapping them in a custom exception is a good idea. For example, if your code reads from some special file private to your component, and the read causes an I/O exception, catching that exception and reporting a custom one is a good idea, because it helps you hide the file operation from the caller.