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 ?