Review of Effective C# Item 15: Utilize using and try-finally for Resource Clean-up

The original Effective C++ Series from Scott Meyers was a real revelation for C++ programmers. It grouped together many idioms from the wildly diverse and complex language and made them understandable. It identified many of the pitfalls and made them avoidable. It was a must read for every serious C++ programmer.

Since then all the major language seems to have an effective series. You would think this was a good idea, but most languages are not as wildly complex as C++, with fewer idioms and pitfalls. They're still there, but the languages have been designed with the idioms in mind, and the introductory texts teach them, and with a lot of the pitfalls already avoided. Consequently most effective series for modern languages are smaller and contain a lot of patterns as well. For example, Effective Java starts off with the builder pattern. To my mind that belongs in a patterns book and it certainly should not be the first Java idiom described.

I am currently reading Effective C# by Bill Wagner. I've got as far as chapter 18 and so far it is full of good advice, but, in my opinion, is extremely poorly explained. Items 6 and 7 cover equality and GetHashCode. These are complex concepts in predominantly reference based languages, like C#, and after I'd finished reading the items I didn't feel I understood them much better.

Items 12 to 20 cover resource management. This is a real passion of mine, so naturally I'm quite critical of what's written here, as well as what's actually written. Luckily most of what's written is sound, but part of Item 15 gives, in my opinion, some just plain bad advice. The following item, 16, is another exceptionally badly written item, all though the advice is sound, but I'll leave that for another time.

Item 15: Utilize using and try-finally for Resource Clean-up Resource management is probably the biggest Achilles heal of garbage collected languages. As such, it should probably be the subject of the first section of any effective series, but item 15 out of 50 isn't to bad.

How and why resources need to be managed in C# is explained satisfactorily by the item, so I won't go over it again. However I was highly amused by one paragraph: "Luckily for you, the C# language designers knew that explicitly releasing resources would be a common task. They added keywords to the language to make it easy." Surely this is treating a symptom, not solving the problem and they should have found a way to encapsulate resource management within types.

My real issue with this item is what the author describes as an ugly construct. There is an implied using example that uses both a SqlConnection and a SqlCommand:

public void ExecuteCommand(string connString, string commandString)
{
    using (var myConnection = new SqlConnection(connString))
    {
        using(var myCommand  = new SqlCommand(commandString, myConnection))
        {
            myConnection.Open();
            myCommand.ExecuteNonQuery();
        }
    }
}

The author points out that you've effectively written this construct:

public void ExecuteCommand(string connString, string commandString)
{
    SqlConnection myConnection = null;
    SqlCommand myCommand = null;

    try
    {
        myConnection = new SqlConnection(connString);
        try
        {
            myCommand = new SqlCommand(commandString, myConnection);

            myConnection.Open();
            myCommand.ExecuteNonQuery();
        }
        finally
        {
            if (myCommand != null)
                myCommand.Dispose();
        }
    }
    finally
    {
        if (myConnection != null)
            myConnection.Dispose();
    }
}

As he finds it ugly, when allocating multiple objects that implement IDispose, he prefers to write his own try/finally blocks:

public void ExecuteCommand(string connString, string commandString)
{
    SqlConnection myConnection = null;
    SqlCommand myCommand = null;

    try
    {
        myConnection = new SqlConnection(connString);
        myCommand = new SqlCommand(commandString, myConnection);

        myConnection.Open();
        myCommand.ExecuteNonQuery();                
    }
    finally
    {
        if (myConnection != null)
            myConnection.Dispose();

        if (myCommand != null)
            myCommand.Dispose();
    }
}

I have two problems with this. The first is that if the Finally For Each Release pattern, as described by Kevlin Henney in Another Tale of Two Patterns, is correctly implemented, the null checks, which are a terrible code smell and often the cause of bugs if they get forgotten, would be completely unnecessary:

public void ExecuteCommand(string connString, string commandString)
{
    var myConnection = new SqlConnection(connString);
            
    try
    {
        var myCommand = new SqlCommand(commandString, myConnection);

        try
        {
            myConnection.Open();
            myCommand.ExecuteNonQuery();
        }
        finally
        {
            myCommand.Dispose();
        }
    }
    finally
    {
        myConnection.Dispose();
    }
}

If the nested try blocks are a problem for you, another method can be introduced:

public void ExecuteCommand(string connString, string commandString)
{
    var myConnection = new SqlConnection(connString);

    try
    {
        ExecuteCommand(myConnection, commandString);
    }
    finally
    {
        myConnection.Dispose();
    }
}

private void ExecuteCommand(SqlConnection myConnection, string commandString)
{
    var myCommand = new SqlCommand(commandString, myConnection);

    try
    {
        myConnection.Open();
        myCommand.ExecuteNonQuery();
    }
    finally
    {
        myCommand.Dispose();
    }
}

However, the real problem is that you are only effectively implementing this construct. If you stick with the original nested using blocks, the compiler creates the construct for you and you don't see it. Which means that it really doesn't matter how ugly it might be and ditching the using blocks and writing your own construct just creates the ugliness. Maybe the root of the authors aesthetic objection is the nesting. Again, this is easily overcome by introducing another function:

public void ExecuteCommand(string connString, string commandString)
{
    using (var myConnection = new SqlConnection(connString))
    {
        ExecuteCommand(myConnection, commandString);
    }
}

private void ExecuteCommand(SqlConnection myConnection, string commandString)
{
    using (var myCommand = new SqlCommand(commandString, myConnection))
    {
        myConnection.Open();
        myCommand.ExecuteNonQuery();
    }
}

Finally
In conclusion, the final part of the summary advice given in the chapter which states, "Whenever you allocate one disposable object in a method, the using statement is the best way to ensure that the resources you allocate are freed in all cases. When you allocate multiple objects in the same method, create multiple using blocks or write your own single try/finally block." should be ignored in favour of "... When you allocate multiple objects in the same method, create multiple using blocks."

A PDF version of this review can be found here.