10

Given that eating exceptions is always bad juju and re-throwing the exception loses the call stack, what's the proper way to re-factor the following?

Eating Exceptions:

try
{
  … do something meaningful
}
catch(SomeException ex)
{
   // eat exception
}
6
  • What do you mean? you wanna ignore it?
    – Aliostad
    Oct 7, 2010 at 17:49
  • @Renegrin: please add more tags describing problem (c# at least).
    – dma_k
    Oct 7, 2010 at 17:50
  • 20
    I vote to change the title of this question to "Eating Exceptions in c# om nom nom "
    – Dan J
    Oct 7, 2010 at 17:58
  • @djacobson: I'm so tempted to do that... Oct 7, 2010 at 18:13
  • 3
    You can´t refactor this. Refactor dont change behavior and throwing exceptions when is not expected is a behavior change. Oct 7, 2010 at 18:22

16 Answers 16

30
try
{
 ...
}
catch(SomeException e)
{
 //Do whatever is needed with e
 throw; //This rethrows and preserves call stack.
}
13
  • 2
    if you are throwing then why are you catching?!
    – Aliostad
    Oct 7, 2010 at 17:50
  • 8
    Edited to be a bit more clear. Essentially showing that you can catch an exception and perform whatever you need to do with the exception and then rethrow while preserving call stack. Oct 7, 2010 at 17:52
  • 1
    +1. Caveat: If you cannot meaningfully handle the exception here, perhaps it is better to simply remove the try/catch, and allow the exception to propagate up the call stack. Oct 7, 2010 at 17:53
  • 3
    @Richard Not a fan of catching/logging then re-throwing, personally think it muddies up the logging since you have no idea what is going to be logged up-stream. It feels very of the moment and that no defined error logic exists. Oct 7, 2010 at 17:54
  • 3
    @Aaron: It never feels ideal, but there are practical reasons (beyond being required to do it by pre-established coding conventions enforced by an existing team). For instance, if you are using remoting, and need to log all exceptions in the business logic on the server, while still propagating exceptions to the client, rather than just do nothing. Then on the client side, the exception is also handled, perhaps in another way, like retrying, displaying the error message, etc.... Oct 7, 2010 at 18:02
7

Catch and handle specific types of exceptions. Good practice is to not just catch System.Exception. A robust routine will strongly type the exceptions it knows how to handle.

Exceptions shouldn't be used for control flow, but there are often specific unwind procedures that need to be taken based on the type of exception.

Depending on the specific type, you may or may not choose to rethrow it. For example, an ASP parsing exception being thrown to an error page that USES the code causing the exception will cause an infinite loop.

try
{

}
catch( FileIOException )
{
    // unwind and re-throw as determined by the specific exception type
}
catch( UnauthorizedAccessException )
{
    // unwind and re-throw as determined by the specific exception type
}
catch( SomeOtherException )
{
    // unwind and re-throw as determined by the specific exception type
}
catch( Exception )
{
   // log and re-throw...add your own message, capture the call stack, etc.

   // throw original exception
   throw;

   // OR, throw your own custom exception that provides more specific detail and captures
   // the original exception as the inner exception
   throw new MyStronglyTypedException();
}
finally
{
     // always clean up
}
2
  • 2
    I think you want to pass the Exception.StackTrace into the MyStronglyTypedException()
    – Woot4Moo
    Oct 7, 2010 at 18:07
  • @Woot4Moo - +1 this is just pseudo code...but thanks for pointing that out so people know.
    – Tim M.
    Oct 7, 2010 at 18:13
6

Most people think it's utterly evil to eat/suppress exceptions, especially with catch-alls. (Ironically, they use the catch all response of "don't use catch-alls, it's evil" :-). I don't understand the religious fervour with which people parrot this view, because if used sensibly, there is nothing wrong with this approach.

  • In my book, the worst case scenario is that my program catastrophically exits -> this creates a very unhappy customer with a total data loss situation. An unhandled exception is guaranteed to cause this every time. So failing to handle an exception is statistically more dangerous than any risk of instability that may occur if an exception is suppressed. In light of this, anything we can reasonably do to protect against an unhandled exception occurring is a good thing.

  • Many people seem to forget that catch alls can often handle any exception correctly, even if they don't know the details of what the exception was. By this I mean that they can guarantee that the program state remains stable, and the program continues to run within its design parameters. Or there may even be side effects such as the user finding a button unresponsive, but they still won't lose any data (i.e. graceful degradation is better than a fatal crash). For example, sometimes you want to return one value on success and a default if you fail for any reason. Part of designing code is knowing when to report errors to the user and when to fix a problem on their behalf so their program "just works". In this situation, a well designed catch-all is often the correct tool for the job.

  • Exceptions worry me. Fundamentally an exception is a guaranteed program crash if I don't handle it. If I only add specific exception handling for the exceptions I expect, my program is inherently fragile. Consider how easily it can be broken:

    • If a programmer forgets to document one exception they might throw, I won't know I need to catch it, and my code will have a vulnerability I'm not aware of.
    • If someone updates a method so that it throws a new exception type, that new exception could ripple up the call stack until it hits my code. But my code was not built to handle the exception. Don't tell me that the libraries I'm calling will never change.
    • Every exception type you specifically handle is another code path to be tested. It significantly multiplies the complexity of testing and/or the risks that a broken bit of handling code might go unnoticed.
  • The view underpinning the "suppression is evil" view is that all exceptions represent an instability or error - but in many cases programmers use exceptions to return little more than status information. For example, FileNotFound. The programmer writing file I/O code has decided on my behalf that a missing file is a fatal error. And it might be. It is up to me to catch this and decide that actually it's a common and perfectly normal, or expected, situation. A lot of the time, suppressing exceptions is necessary to simply stop someone else's "decision" taking out my application. The old approach of simply ignoring error return codes wasn't always a bad thing, especially given the amount of effort it takes to catch and suppress the myriad "status" exceptions that are bandied about.

The trick to silently eating/suppressing exceptions is just to be sure that this is the correct way to handle them. (And in many cases, it's not). So there may be no need to refactor your example code - it might not be bad juju.

3

That all depends on where the code lives.

In the depths of the system? If that is the case then I would gather some form of standard error handling should exist across the product, if not it needs to.

If it is on the presentation side for instance it may have no value to anyone except the code, and in that case additional logic may need to be placed in a finally block.

Or let it roll up hill altogether and don't wrap it in a try catch if you aren't going to do anything useful in the catch anyways.

… do something meaningful
2

To add to the excellent comments already provided.

There are three way to "re-throw" an exception:

catch (Exception ex)
{
   throw;
}

The above preserves the call stack of the original exception.

catch (Exception ex)
{
   throw ex;
}

The above eats the original exception chain and begins a new one.

catch (Exception ex)
{
   throw new MyException("blah", ex);
}

The above adds the original exception to the InnerException of a new chain. This can be the best of both worlds, but which one is correct is highly dependent on what you need.

2

Your code can be rewritten (to eat exception) like this

try
{
  … do something meaningful
}
catch
{
   // eat exception
}

But I don't understand what you want to do by refactoring!!

Edit:

Re-throwing using throw; doesn't work always. Read this -> http://weblogs.asp.net/fmarguerie/archive/2008/01/02/rethrowing-exceptions-and-preserving-the-full-call-stack-trace.aspx

2
  • First I'm removing the warning in vs2008. Sencond trying to do the proper action with the exception. Throw seems effective so far. Oct 7, 2010 at 18:34
  • Well, if you are happy with "works so far" ways.. I'll say no more. :)
    – Nayan
    Oct 8, 2010 at 4:48
2

In general, it's not a good idea to catch the general Exception unless you can actually handle it. I think the right answer is a combination of Tim's and Joshua's answers. If there are specific exceptions that you can handle and remain in a good state, for example FileNotFoundException you should catch it, handle it, and move on, as seen here:

try
{
    // do something meaningful
}
catch(FileNotFoundException)
{
    MessageBox.Show("The file does not exist.");
}

If you can't handle it and remain in a good state, don't catch it in the first place.

However, one case where you would want to catch the general Exception and re-throw it would be if you have any cleanup that you will need to do, for example aborting a database transaction, before the exception bubbles up. We can accomplish this by extending the previous example like so:

try
{
    BeginTransaction();

    // do something meaningful

    CommitTransaction();
}
catch(FileNotFoundException)
{
    MessageBox.Show("The file does not exist.");
    AbortTransaction();
}
catch(Exception)
{
    AbortTransaction();
    throw;                  // using "throw;" instead of "throw ex;" preserves
                            // the stack trace
}
1

Refactor it to:

// No try
{ 
   … do something meaningful 
} 
// No catch

and let the exception be handled at the main loop.

0

if the catch() block only rethrows exception and does not do any real exception handling then you don't need try..catch at all.

1
  • The OP's catch block does not rethrow the exception.
    – Brian
    Oct 7, 2010 at 18:03
0

Part of the problem with eating exceptions is that it's inherently unclear what they're hiding. So... the question of the proper refactoring isn't easily answered. Ideally, however, you'd remove the try...catch clause entirely; it's unnecessary in most cases.

Best practice is to avoid try...catch entirely wherever possible; if you must deal with exceptions, then do so as locally and specifically as possible and don't propagate them up the stack; finally, include a global unhandled exception handler that does the appropriate logging (and perhaps offers to restart the application if necessary).

2
  • This is the reason I posted this thread. I felt it's very expensive sometimes to implement the try..catch, yet in the code I'm refactoring it's littered with them and its swallowing the exceptions. Oct 7, 2010 at 18:43
  • Well, in that case you're stuck: you can be almost certain that simply removing the try...catch clauses will cause unhandled exceptions; but further development is hindered by the difficulty in getting a grip on bugs. Do you have decent unit tests? Oct 8, 2010 at 7:06
0

Unless the catch block actually does something with the exception (e.g., logging it to a system error file), there is no need to even have the try/catch block.

That being said, if the exception is worth informing the user about (e.g, logging it), then by all means use a catch block to do so.

One particularly bad pitfall of ignoring exceptions is that certain (fatal) exceptions should cause the program to terminate. Such exceptions (e.g., failure to load a class) leave the program in an unstable state, which will only lead to disaster later on in the execution. In these cases, logging the exception and then gracefully terminating is the only reasonable thing to do.

0

The particular way in which exceptions are eaten is not important. Never eat exceptions by any means!

Only catch exceptions that are expected to occur and which you can do something about. Examples of this include file and network IO, security exceptions, etc. For those cases you can display an explaination of what happened to the user, and sometimes you can recover gracefully.

Do not catch exceptions that should never occur. Examples of these are null-reference exceptions, invalid operation exceptions, etc. The code should be written so that these exceptions will never happen, so there is no need to catch them. If those exceptions are happending, then fix the bugs. Don't swallow the exceptions.

It is OK to log all exceptions, but this should be done with the unhandled exception handler on the program and any threads that are created. This is not done with a try/catch.

0

You can rethrow exception without losing call stack just re-throw as

catch(Exception e)
{
  throw;
}

Why would you need this? Usage example: Somewhere in your app you have 3rd party code and you wrap it, and if it throws exceptions you throw WrappingException.

When you execute some other code you might get exception either from 3rdparty or either from your own so you may need:

try
{
//code that runs 3rd party
//your code, but it may throw Null ref or any other exception
}
catch( WrappingException)
{
 throw;
}
catch( Exception e)
{
 throw new MyAppLayer3Exception("there was exception...", e);
}

In this case you do not wrap WrappingException with your MyAppLayer3Exception.

So, at the top level of you application you may catch all exceptions and by knowing Type of exception you will know where from it came!

Hope it helps.

0

eating exceptions is not always "bad juju". There is no magic here; just write code to do what you need to do. As a matter of hygiene, if you catch an exception and ignore it, comment as to why you are doing it.

try
{
 .....
}
catch (something)
{
  // we can safely ignore ex becuase ....
}
0

Sometimes, it's just best not to deal with exceptions if you really don't want to deal with the added responsibility that comes with exceptions. For example, rather than catching an NullReferenceException, why not just make sure that the object exists before you try to do something with it?

if (yourObject != null)
{
    ... do something meaningful with yourObject ...
}

Exceptions are best reserved to handle those things you really have no control over, such as the sudden loss of a connection, or things which have the potential to kill a long-running process, such as a data import. When an exception is thrown, regardless of the reason, your application has reached a point of instability. You catch the exception to return the application to a point of stability by cleaning up the mess, e.g. disposing of the lost connection and creating a new one OR logging the line where the error occurred and advancing to the next line.

I've been dealing with exception handling for the last 15 years, starting with the first six versions of Delphi, up to (and including) .NET 1.0-4.0. It is a powerful tool, but it is a tool that is often overused. I have found consistently, during that time, the most effective exception handling process is deferring to if-then before try-catch.

4
  • Beware this thought process in the real (multithreaded) world. If you test (yourObject) and it is non-null, you will enter the if statement. At which point another thread can interrupt you and set (yourObject) to null, and you will still get a NullReferenceException, only now you won't be expecting it because the code 'looks' safe. Oct 7, 2010 at 19:33
  • @Jason: You are correct...with that in mind, I believe we can both agree that potential threading conflicts would clearly fall into the category of "Things You Really Have No Control Over", especially if you haven't grasped the concepts of locking and synchronization. Then again, if you haven't grasped those concepts, then maybe you should reconsider multithreading in the first place...
    – Neil T.
    Oct 8, 2010 at 6:51
  • Indeed, but in a multithreaded app, the 'if' approach looks harmless but won't work reliably without syncs/locks. Conversely a catch() will work without needing syncs/locks. A catch is easier to write than a lock, so (much as I hate to admit it, because I agree with your stance about exceptions often being misused) the exception handling approach may be a lot cleaner & easier under these circumstances. Oct 8, 2010 at 20:42
  • @Jason: Under these circumstances, I agree with you.
    – Neil T.
    Oct 9, 2010 at 5:57
0

One major problem with the exception hierarchy is that exceptions are categorized based upon what happened, rather than based upon the system state. Some exceptions mean "a function couldn't perform its task, but it didn't disturb the system state either". Others mean "Run for your lives! The whole system is melting down!" In many cases, it would be entirely proper for a routine which could handle the failure of a called method to swallow any and all exceptions of the former type; in other cases, such exceptions should be rethrown in a manner which indicates possible state corruption (e.g. because there was a failure in an operation necessary to reset the system state; even though the attempt to perform that operation didn't disturb anything, the fact that the state wasn't reset means it's corrupted).

It would be possible for one to manage one's own exceptions into such a hierarchy, but I don't know any good way to deal with other exceptions.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.