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:\Documents\Documents\Blog material\Throw\ThrowExample\ThrowExample\Program.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:\Documents\Documents\Blog material\Throw\ThrowExample\ThrowExample\Program.cs:line 8
at ThrowExample.Program.Main() in D:\Documents\Documents\Blog material\Throw\ThrowExample\ThrowExample\Program.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:\Documents\Documents\Blog material\Throw\ThrowExample\ThrowExample\Program.cs:line 8
at ThrowExample.Program.Main() in D:\Documents\Documents\Blog material\Throw\ThrowExample\ThrowExample\Program.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:\\Documents\\Documents\\Blog material\\Throw\\ThrowExample\\ThrowExample\\Program.cs:line 20
and the inner exception stack trace would be
at ThrowExample.Program.ThrowException() in D:\Documents\Documents\Blog material\Throw\ThrowExample\ThrowExample\Program.cs:line 8
at ThrowExample.Program.Main() in D:\Documents\Documents\Blog material\Throw\ThrowExample\ThrowExample\Program.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?