Exception handling – be smart about it !

I’ve encounter a numerous bad usages of try, catch and throw statements in my last 3 years in .NET so I thought to write here my “best practices” in this subject.

Before I begin, just to remind you about the “using” keyword

” The using statement defines a scope at the end of which an object will be disposed. ” (MSDN)

Meaning, this code:

using (MyDisposableObject o = new MyDisposableObject())
{
// use o…
}

Is equal to:

MyDisposableObject o;
try
{
   o = new MyDisposableObject();
   // use o…
}
finally
{
   // Don’t forget, MyDisposableObject must implement IDisposable
   o.Dispose();
}

The using statement is much more “cleaner” than the try-finally(->call Dispose) block. Of course, in order to use the using statement, MyDisposableObject must implement IDisposable interface, but most of the .NET frameworks’ classes which use external resources do, so no problem here.

When do I use the using keyword instead of “try-catch(-finally)” ?

In any case your code block doesn’t required any exception handling (catch) and you’re using a disposable object.

When do I catch an exception ?

You should use the catch statement only if you can REALLY handle the exception – meaning you want\need to “eat”(catch it and do nothing about it) the exception or you (want to)\can try to “fix” the application’s flow according the exception type (call transactionInstance.Rollback() in my data-access-layer if an error occurred, for example).

Do NOT catch exceptions as a “default” behavior in your code !

The following code is a BAD practice in exception handling:

try
{
   //some code
}
catch(Exception e)
{
   throw new Exception(“X operation error: “ + e.Message);
}
finally // if exists.
{
   //some code
}

Why is it bad ? The catch statement doesn’t handling the original exception, it creates a (bad)new one which means:

The Stack Trace of the original exception will be LOST, which means I lose the ability to view the entire “process” (who called to who flow).
In the demonstrated code, I catch an exception and re-throwing a pointless new exception. Throwing exceptions is an expensive task so you should avoid (at any cost) throwing them as long as you don’t really need to !
If you wrap an exception, at least save the original exception in the InnerException property (I’ll elaborate later on).

When do I wrap an exception, when do I rethrow it ?

You should catch and wrap the exception with a new one only if you can add INFORMATIVE data to the original exception which WILL be used later on. Writing this type of code (in my DAL) will be a smart idea usually:

SqlTransaction trans;
SqlConnection conn = null;
try
{
    // use the connection to do some DB operation

    trans.Commit();
}
catch(Exception e)
{
    if (trans != null)
        trans.Rollback();

    // Wrap the exception with DALException
    // I can check if e is SqlException and by the e.Number –
    // Set a “clean”(show to user) message to the DALException.
    // I can add the full “sql” exception in some custom property,
    // I can determine which stored procedure went wrong,
    // I can determine the line number (and so on).
    throw new DALException(“clean message to the user”, e);
}
finally
{
    if ( conn != null && conn.State == ConnectionState.Open )
        conn.Close();
}

Why is this code smart ? Because I call the Rollback() in case of an error, which will ensure “clean” database. Because it “hides” the original SqlException which allows me, at my Business Layer, to catch a generic DALException which will abstract the Business Layer from the Data Access Layer. Because I CAN add more informative data to the exception so the Business Layer could send the GUI (to show the user).
You should rethrow the exception if you catch-ed it but you “found out” that you can’t really handle it:

try
{
    // do some code…
}
catch(Exception e)
{
    if (e is SqlException)
    {
        // Add more information about the exception and
        // throw a new, more informative, exception:
        throw new DALException (“more data”, e);
    }
    else
    {
        // I can’t handle it really, so I’m not even going to try
        throw; // <– look at this syntax ! I’ll explain later on
    }
}

Calling throw; will bubble the original exception (including its’ Stack Trace) – this will actually “cancel” the catch statement.

When you wrap an exception, you should *almost* always use the “InnerException” property

When you wrap an exception, you should save the original exception as InnerException:

try
{
}
catch(Exception e)
{
   throw new MyCustomException(“custom data”, e);

   // OR

   MyCustomException mce = new MyCustomException(“custom data”);
   mce.InnerException = e;
}

This will preserve the original stack which will be important for later debugging.

Any Insights you want to share with me ?

2 thoughts on “Exception handling – be smart about it !”

Shani - The Human-Debugger says:

September 13, 2005 at 11:45 pm

hi,
i love this post,
i think you should add here this kind of catch:

try
{
// do something realy important like
// launching missiles
}
catch // <– look at this strange catch :)
{
// do something like poping up a message box.
}

why should we use this thing?
for example…
if you want to "eat" the exception and show standard message which not uses the exception details.

love u,
ronaldinho.
Oren Ellenbogen says:

September 14, 2005 at 1:50 pm

Shani (aka Ronaldinho), did I mention you’re a crazy dude already ?

You’re right, this pattern should be used if (1) you want to eat any kind of exception and not publish it to some log (this is a bad practice in my opinion, so if you choose to do it, write a meaningful remark to explain your selection) or (2) you want to show a "all-around-message" no matter what.

Another bad-warning-causing example I’ve seen is A LOT:
try
{
// some code…
}
catch(Exception err)
{
}
// (finally sometimes)

This will obviously "eat" the exception BUT it will also generate a warning about the "err … is never used". If I see it in one more application I need to maintain I’ll kill someone (me, probably) !

The fixed code should look like:
try
{
// some code…
}
catch(Exception) // <– catch the excpetion type, but don’t "assign" it to a "real" variable.
{
}
// (finally sometimes)

I’ll say it only once: WARNING IS EQUAL TO ERROR !