Gotcha – Improper exception handling

I was reading today a document written by Juval Lowy from IDesign "C# Coding standard - Guidelines and Best Practices V2.2" and under No 17 rule in Coding Practices I found next thing:

17. In a catch statement that throws an exception, always throw the original exception (or another exception constructed from the original exception) to maintain the stack of original error:

catch (Exception exception)
{MessageBox.Show(exception.Message);

throw exception;
}

I think this definitely is not a best practice, and I'll try to explain why:

throw ex

The problem this practice tries to solve is that in case of rethrowing of exception in catch block stack pointer moves from the original exception line to line of rethrowing of exception

   1: using System;

   2: namespace ThrowExample

   3: {

   4:     internal class Program

   5:     {

   6:         public static void ThrowException()

   7:         {

   8:             throw new NotImplementedException("Exception thrown!"); //Line 8

   9:         }

  10:  

  11:         private static void Main()

  12:         {

  13:             try

  14:             {

  15:                 ThrowException(); //Line 15

  16:             }

  17:             catch (Exception e)

  18:             {

  19:                 Console.WriteLine(e.Message);

  20:                 throw e; //Line 20

  21:             }

  22:         }

  23:     }

  24: }

Stack result would be

"Exception thrown!"

at ThrowExample.Program.Main() in D:DocumentsDocumentsBlog materialThrowThrowExampleThrowExampleProgram.cs:line 20

at System.AppDomain.nExecuteAssembly(Assembly assembly, String[] args)
...

That's why IDesign suggest MessageBox printing of stack trace before it is been destroyed, but this is bad practice because it is:

• Approach tied to specific UI technology (what with ASP NET app?)
• Disables loging of exception through Log4Net  or EntLib block
• Relies on developer to read the message and makes some action (when he clicks ok button all traces of the exception log are disappearing)

throw;

Instead of MessageBox printing the proper way is to use throw; call which doesn't destroy stack trace.

So our example. enhanced with throw; would be:

   1: using System;

   2: namespace ThrowExample

   3: {

   4:     internal class Program

   5:     {

   6:         public static void ThrowException()

   7:         {

   8:             throw new NotImplementedException("Exception thrown!"); //Line 8

   9:         }

  10:  

  11:         private static void Main()

  12:         {

  13:             try

  14:             {

  15:                 ThrowException(); //Line 15

  16:             }

  17:             catch

  18:             {

  19:                 Console.WriteLine("Exception happened");

  20:                 throw; //Line 20

  21:             }

  22:         }

  23:     }

  24: }

and then stack would be

"Exception thrown!"

at ThrowExample.Program.ThrowException() in D:DocumentsDocumentsBlog materialThrowThrowExampleThrowExampleProgram.cs:line 8

at ThrowExample.Program.Main() in D:DocumentsDocumentsBlog materialThrowThrowExampleThrowExampleProgram.cs:line 20

at System.AppDomain.nExecuteAssembly(Assembly assembly, String[] args)
...

no try - catch

But there is one more problem: In "throw;" case of we are not seeing in stack trace line 15 which we would see in case we didn't have try-catch-block.

   1: using System;

   2: namespace ThrowExample

   3: {

   4:     internal class Program

   5:     {

   6:         public static void ThrowException()

   7:         {

   8:             throw new NotImplementedException("Exception thrown!");

   9:         }

  10:  

  11:         private static void Main()

  12:         {

  13:             /*            try

  14:             {*/

  15:             ThrowException();

  16:             /*}

  17:             catch

  18:             {

  19:                 //Console.WriteLine(e.Message);

  20:                 throw; //Line 20

  21:             }        */

  22:         }

  23:     }

  24: }

and then stack would be

"Exception thrown!"

at ThrowExample.Program.ThrowException() in D:DocumentsDocumentsBlog materialThrowThrowExampleThrowExampleProgram.cs:line 8

at ThrowExample.Program.Main() in D:DocumentsDocumentsBlog materialThrowThrowExampleThrowExampleProgram.cs:line 15

at System.AppDomain.nExecuteAssembly(Assembly assembly, String[] args)
...

The way how we can see all of the important stack exception data is next one:

throw ex + inner exception

   1: using System;

   2: namespace ThrowExample

   3: {

   4:     internal class Program

   5:     {

   6:         public static void ThrowException()

   7:         {

   8:             throw new NotImplementedException("Exception thrown!"); // Line 8

   9:         }

  10:  

  11:         private static void Main()

  12:         {

  13:             try

  14:             {

  15:                 ThrowException(); // Line 15

  16:             }

  17:             catch (Exception e)

  18:             {

  19:                 Console.WriteLine(e.Message);

  20:                 throw new Exception("Some message", e); //Line 20

  21:             }

  22:         }

  23:     }

  24: }

the main stack result would be

"Exception thrown!"

at ThrowExample.Program.Main() in D:DocumentsDocumentsBlog materialThrowThrowExampleThrowExampleProgram.cs:line 20

and the inner exception stack trace would be

at ThrowExample.Program.ThrowException() in D:DocumentsDocumentsBlog materialThrowThrowExampleThrowExampleProgram.cs:line 8
at ThrowExample.Program.Main() in D:DocumentsDocumentsBlog materialThrowThrowExampleThrowExampleProgram.cs:line 15

So only on this way we could get all the data needed

Therefore, I believe that only the last approach (maybe the throw; approach could be ok)  is the approach which deserves to be described as best practice

Don't you think so?

Share this post :