OK, so the other day I posted up about coding standards and some of their draw backs, and I mentioned how CODE REVIEWS can often have a better outcome. I also suggested how the code review process can become a little bit more painless by using tools like a Wiki to create hyperlinked knowledge bases.
I'm currently in the process of setting up FlexWiki on this site, but in the meantime I thought I would post up an initial example of what I though a good re-usable code review comment would look like.
Context:
You have written some code that makes use of a scarce (and quite often pooled) resource like the System.Data.SqlClient.SqlConnection class.
SqlConnection connection = null;
try
{
connection = new SqlConnection(...);
// Do stuff with connection.
}
finally
{
connection.Dispose();
}
Your code is quite good, you are creating and using the connection within the context of a try-finally block, this means if anything goes wrong you can clean up the database connection.
Problem:
While the code is good, it does have a minor bug which many not manifest itself until your system is under load. By default, the SqlConnection class draws off a pool of existing connections which can grow to a maximum of one hundred connections. Under extreme load conditions it is possible that you may exhaust the connection pool resulting in the the construction call for the SqlConnection object failing (leaving connection equal to null).
An exception will be raised and the code in the finally block will be executed (regardless of the failure or not) but because connection is null a NullReferenceException will be thrown when the code in the finally block attempts to call Dispose().
While the result is the same (an exception is thrown), this code masks the exception that could provide important tips to developers or administrators trying to debug the cause of the problem.
Solution:
The solution to this problem is simple, check for null on objects created inside try-finally blocks when there is a risk of construction not succeeding because of a given situation. The same rule applies of objects that may have been initalised AFTER the failed construction call.
SqlConnection connection = null;
try
{
connection = new SqlConnection(...);
// Do stuff with connection.
}
finally
{
if (connection != null)
{
connection.Dispose();
}
}