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.

Answer 4

It's completely OK. You don't have to catch each exception type separately. You can catch specific type of exception, if you want to handle it in specific way. If you want to handle all exceptions in same way - catch base Exception and handle it as you do. Otherwise you will have duplicated code in each catch block.

Answer 5

This is what I usually say about exception handling:

• The first thing to do with exceptions is ... nothing. There will be a high level catch all handler (like the yellow ASP.NET error page) that will help you, and you will have a full stack frame (note this is not the case in other non .NET environments). And if you have the corresponding PDBs around, you will also have the source code line numbers.
• Now, if you want to add some information to some exceptions, then of course you can do it, at carefully chosen places (maybe places where real exceptions actually happened and you want to improve your code for future errors), but make sure you really add value to the original one (and make sure you also embark the original one as the inner exception, like you do in your sample).

So, I would say your sample code could be ok. It really depends on the "Custom error message" (and possibly exception custom properties - make sure they are serializable). It has to add value or meaning to help diagnose the problem. For exemple, this looks quite ok to me (could be improved):

string filePath = ... ;
try
{
    CreateTheFile(filePath);
    DoThisToTheFile(filePath);
    DoThatToTheFile(filePath);
    ...
}
catch (Exception e)
{
    throw new FileProcessException("I wasn't able to complete operation XYZ with the file at '" + filePath + "'.", e);
}

This doesn't:

string filePath = ... ;
try
{
    CreateTheFile(filePath);
    DoThisToTheFile(filePath);
    DoThatToTheFile(filePath);
}
catch (Exception e)
{
    throw new Exception("I wasn't able to do what I needed to do.", e);
}

Answer 6

Foreword: I consider this pattern to be usable only in certain conditions and with full understanding of its good and bad sides.

---

Statement 1: An exception is when a member fails to complete the task it is supposed to perform as indicated by its name. (Jeffry Richter, CLR via C# Fourth Edition)

Statement 2: Sometimes we want from a library member to a) Get a result or b) Tell us it's impossible and we don't care about all the details, we'll just send details to the library developers.

Conclusion from St.1, St.2: When implementing a library method we can wrap a general Exception and throw a Custom one, including the source one as its InnerException. In this case a developer using this member will need to catch just one exception and we'll still get debug information.

---

Case 1: You are implementing a library method and don't want to expose it's internals or you intend/assume to change it's internals in future.

public string GetConfig()
{
    try
    {
        var assembly = Assembly.GetExecutingAssembly();
        var resourceName = "MyCompany.MyProduct.MyFile.cfg";

        // ArgumentNullException
        // ArgumentException
        // FileLoadException
        // FileNotFoundException
        // BadImageFormatException
        // NotImplementedException
        using (Stream stream = assembly.GetManifestResourceStream(resourceName))
        // ArgumentException
        // ArgumentNullException
        using (StreamReader reader = new StreamReader(stream))
        {
            // OutOfMemoryException
            // IOException
            string result = reader.ReadToEnd();
        }
        return result;

        // TODO: Read config parameter from DB 'Configuration'
    }
    catch (Exception ex)
    {
        throw new ConfigException("Unable to get configuration", ex);
    }
}

It's a pretty solid code and you are sure it won't throw an exception ever. Because it's not supposed to. Are you sure? Or you'd wrap it into try-catch, just in case? Or you'd make a developer do this job? What if he doesn't care whether this method will succeed, may be he has a backup plan? May be he'll wrap call to this method to try-catch(Exception e) instead of you? I don't think so.

Pros:

1. You are hiding implementation details and are free to change it in future;
2. The callers don't have to catch whole bunch of different exceptions, if they care, they can look at the InnerException.

Cons:

1. You are losing stack trace (It may be not that important for a third-party library);

---

Case 2: You want to add information to exception. It's not directly the code you wrote in the question, but it's still catching a general Exception and I think this is important underestimated part of the Exception Handling facility.

catch (Exception ex)
{
    ex.Data.Add(paramName);
    throw;
}

---

Addition: I would edit your pattern in the following way:

try
{
    // code that can cause various exceptions...
}
catch (Exception e)  
{
    if (e is ThreadAbortException || e is StackOverflowException || e is OutOfMemoryException)
    {                                       
        throw;
    }

    throw new MyCustomException("Custom error message", e);
}

---

Summary: This pattern can be used when developing a library public method to hide implementation details and simplify its using by developers.

Answer 7

The general principal with Exceptions is to catch them and handle them locally if applicable and otherwise allow them to bubble up to the top level container.

It all depends on the context that your application is running in. For example if the application is an ASP.Net web application then certain exceptions can be handled by the ASP application server in IIS but expected application specific errors (ones that are defined in your own interfaces) should be caught and presented to the end user if appropriate.

If you are dealing with I/O then there are a lot of factors that are beyond your control (network availability, disk hardware, etc.) so if there is a failure here then it is a good idea to deal with it straight away by catching the exception and presenting the user with and error message.

Most importantly don't fail silently so don't wrap the exceptions in your own exception and then ignore them. Better to allow them to bubble up and find them in the web server log for example.

Answer 8

After reading the question, answers and the comments posted, I think a thorough answer by Iain Galloway could explain much of it in broader way.

My short and sweet point to explain it,

1. The exceptions should be caught and custom exception should only be thrown when you want to hide the technical details from the end user and want only user to get informed that something failed with some proper message but on the other hand would always log the same exception to the log file so that we can provide some technical assistance and help to the user if the same scenario occurs frequently and from the log file we get the information that we need (technically).
2. You explicitly catch some exception type and know the exact scenario in what case the exception is thrown by the method and then handle it and have some predefined codes that can explain the error in the other methods calling the function and have some actions to take on that error codes.
   This could rather help if you call many functions and they all throw some pre-defined exception with exception codes which can help categorizing them and take some actions accordingly.

In some other comment you mentioned, what about system critical exceptions like OutOfMemoryException you would only be able to catch those exception if you have explicitly added, [HandleProcessCorruptedStateExceptions] section over the function and to get detail answer in what scenario you should handle it read this SO post

Answer 9

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

Answer 10

Depends on what you have to do with when exception arises. If you wish to globally handle all the exceptions, that's OK to re-throw the exception to the caller method and let it handle the exceptions. But there are many scenarios where exceptions must be handled locally, in such cases, catching exception with known types should be preferred and a last catch should catch Exception ex (to catch an unexpected exception) and re-throw the exception to caller.

try
{
    // code that can cause various exceptions...
}
catch (ArithmeticException e)
{
    //handle arithmetic exception
}
catch (IntegerOverflowException e)
{
    //handle overflow exception
}
catch (CustomException e)
{
    //handle your custom exception if thrown from try{} on meeting certain conditions.
}
catch (Exception e)
{
    throw new Exception(e); //handle this in the caller method
}

Answer 11

It might be good practice or might not depending on the scenario. Though I am not an expert here, to my knowledge I consider doing this is fine when you are not sure about exception. I see you are considering a code in try that can throw multiple exceptions. If you are not sure about which exception can be thrown, it is always better to do it the way you have done it. Anyway the execution will not allow you to execute multiple catch blocks. Whenever you hit first catch block, control will go to finally or wherever next you want it to go but definitely not another catch block.

Answer 12

First universal rule - Never swallow exceptions !! When you write a piece of code, you should be able to predict most of the cases where there could be an exception, and requires to be notified, such cases should be handled in the code. You should have a general logging module, that logs any exception caught by the application, but might not be of any use to the user actually.

Show the errors that are relevant to the user, for example the user of the system might not understand IndexOutOfRange exception, but it might be an interesting subject for us to research on why this occurred.

We need to always know what went wrong and when so that we could analyze the root cause and prevent it from happening again, because if it could happen once. It will happen again and may be it could cause a disaster. The only way to find what is wrong is by logging in what ever error the application encounters.

Answer 13

You can use ellipses too.

   catch (...) 
   {
        // catches all exceptions, not already catches by a catch block before
        // can be used to catch exception of unknown or irrelevant type
   }

Except this what you can do is nested try-catch

try
{
    //some code which is not for database related 
    try
    {
      //database related code with connection open
    }
    catch(//database related exception)
    {
        //statement to terminate
    }
    **finally()
    {
        //close connection,destroy object
    }**
}
catch(//general exception)
{
    //statement to terminate
}

According to me, this would help you to get more concise idea of your error type.

Answer 14

I think that this issue is very speculative, it often depends on specific case, but I did some research and I will share it. First, I want to express my opinion on code from lain Galloway:

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);
}

If GetDataFromNetwork could throw FormatException, than this external calling should has own method, in which will be handled that exception and it should be converted into custom exception like here:

try {
  GetDataFromNetwork();
} catch (FormatException ex) {
  // here you should wrap exception and add custom message, which will specify occuring problem
}

When I´m creating custom exception for specific application, I extend MyGeneralException from Exception and every more specific exception will extend MyGeneralException. So, at the moment you are wrap into custom exception, you should then put in the method throws MyGeneralException.

I´m using rule, which I took over from more experienced developers than I am, that at the first place, when could be thrown some foreign exception, there it should be wrappped into custom, because you don´t want to be dependent on the other module´s exception.

Then If you will use method anywhere, you will put only MyGeneralException into method signature throws and it will bubble up through the layers of application. It should be catched and processed at the highest level, mostly exception message is used to creating response by some handler, or it could be handled manually.

Mainly during designing exception handling there should be considered, if your library will use third party developers, they are not interest in processing many exceptions.