How quickly should I close a using block?

Question

A question came up during a code review the other day about how quickly a using block should be closed. One camp said, 'as soon as you are done with the object'; the other, 'sometime before it goes out of scope'.

In this specific example, there are a DataTable and a SqlCommand object to be disposed. We need to reference both in a single statement, and we need to iterate the DataTable.

Camp 1:

List<MyObject> listToReturn = new List<MyObject>();
DataTable dt = null;
try
{
    using (InHouseDataAdapter inHouseDataAdapter = new InHouseDataAdapter())
    using (SqlCommand cmd = new SqlCommand())
    {
        dt = inHouseDataAdapter.GetDataTable(cmd);
    }

    foreach (DataRow dr in dt.Rows)
    {
        listToReturn.Add(new MyObject(dr));
    }
}
finally
{
    if (dt != null)
    {
        dt.Dispose();
    }
}

Reasoning: Dispose the SqlCommand as soon as you are done using it. Don't start potentially long operations, such as iterating the table, within another object's using block.

Camp 2:

List<MyObject> listToReturn = new List<MyObject>();
using (InHouseDataAdapter inHouseDataAdapter = new InHouseDataAdapter())
using (SqlCommand cmd = new SqlCommand())
using (DataTable dt = inHouseDataAdapter.GetDataTable(cmd))
{
    foreach (DataRow dr in dt.Rows)
    {
        listToReturn.Add(new MyObject(dr));
    }
}

Reasoning: This code is much cleaner. All objects are guaranteed to be disposed no matter what, and none are really resource-intensive, so it is not important to dispose any immediately.

I'm in Camp 2. Where are you, and why?

Edit: Several people have pointed out that DataTable need not be disposed (see Corey Sunwold's answer) and that Camp 1's original example is uglier than it needs to be. Here are some revised examples that also take into account the fact that most of the time, I have to set some properties on the SqlCommand. If anyone has seen or can think of a better example to support either position, please share it.

Camp 1, version 2:

DataTable dt = null;
using (InHouseDataAdapter inHouseDataAdapter = new InHouseDataAdapter(_connectionString))
using (SqlCommand cmd = new SqlCommand("up_my_proc"))
{
    cmd.CommandType = CommandType.StoredProcedure;
    cmd.Parameters.Add("@class_id", 27);
    dt = inHouseDataAdapter.GetDataTable(cmd);
}

foreach (DataRow dr in dt.Rows)
{
    listToReturn.Add(new MyObject(dr));
}

Camp 2, version 2:

using (InHouseDataAdapter inHouseDataAdapter = new InHouseDataAdapter(_connectionString))
using (SqlCommand cmd = new SqlCommand("up_my_proc"))
{
    cmd.CommandType = CommandType.StoredProcedure;
    cmd.Parameters.Add("@class_id", 27);
    DataTable dt = inHouseDataAdapter.GetDataTable(cmd);
    foreach (DataRow dr in dt.Rows)
    {
        listToReturn.Add(new MyObject(dr));
    }
}

I think most people will agree that the readability argument is now much reduced, and that this is not the best example of what I am trying to ask. (That is especially true once I tell you that the SqlConnection is closed before the GetDataTable() method exits, and there is no measurable performance difference for the data used in this instance.) If I may add to my question this late, are there instances where it does make a difference whether I dispose the object immediately? For example, as Gregory Higley mentioned, a shared resource like an OS handle.

Edit: (Explaining my choice of answer) Many thanks to all who contributed opinions, examples, and other helpful feedback! We seem to be split about evenly, but what stands out from everyone's answers is the idea that 'camp 1 is definitely right, but depending on the object, camp 2 may be okay'. I meant this to be a general discussion of disposal of all types of objects, but I chose a bad example to illustrate it. Since much of the discussion focused on that particular example, I have chosen the answer that gave me important information about the specific objects in use, and proved that I need to consider each object carefully when making this kind of decision. (It would be difficult to choose a 'best answer' to a question as vague as what is in my title, anyway.) Future readers with the same dilemma, please see all the answers below, as many of them raise interesting points.

Answer 1

Mostly I agree with camp 1. However, you should note its probably not necessary to dispose a DataTable.

Answer 2

I think it depends upon what kind of unmanaged resources are held by the disposable object. Overall though, I'm with you in Camp 2.

If the unmanaged resource is shared in some way, particularly if it represents an operating system handle of some kind, I'd try to dispose of it as quickly as possible.

Answer 3

I am in Camp 2 as you do. Sometimes criticality of the resources are determined by available resources on the machine. Camp 2 solution does believe in preventive measure where remove the objects as you are done rather than the lazy Camp 1.

Answer 4

We usually go with camp 2 - for the reason that the outside using statement uses a SqlTransaction object.

We need the transaction to be disposed at the end so if an exception is thrown when using the data reader, the tranaction can be rolled back.

Answer 5

The Camp2 is more readable, so I would prefer it over Camp1 in most cases. The more readable your code is, the less pain you get in supporting it.

In some rare cases I would prefer Camp1 if there would be clear need to dispose resource very very quickly. I mean that if you encountered some problems disposing connection a bit later. But in most cases there wouldn't be any penalty if you go the Camp2 way.

Answer 6

It all comes down to how long do you think it is reasonable to hold onto an external resource. Data connections arent for free, so it makes sense to clean them up as soon as possible. For readability I would still maintain my stance as it is very clear as to what is the resource retaining objects here.

But in reality this is a bit of a purest question since we are talking low milliseconds here. I would say err on the side of camp 1, especially if you have a long running use of the data queried.

Also I would get rid of the DataTable clean-up. Its not needed on a managed object with no resource references. And in doing so pretty much negates the readability argument.

Answer 7

+1 for Camp 2.

Your Camp 1 sample disposes objects that could be used AFTER beeing disposed. I think it is not a problem with your exact scenario, but could be problem with other cases. Camp 2 version forces you to dispose objects in properly nested scopes making code safe.

Sample:

StreamWriter writer;
using(var stream = new FileStream(name))
{
    writer = new StreamWriter(stream);
}
writer.Write(1); // <= writnig to closed stream here.
writer.Dispose();

Answer 8

Your Camp 1 example is a bit of a Straw Man, it doesn't have to be as ugly as that.

If performance was an issue (which is probably rare, and unlikely in this contrived example), I would go for a cleaner version of "Camp 1" by refactoring the generation of the DataTable into a method:

private DataTable GetDataTableFromInHouseAdapter()
{
    using (InHouseDataAdapter inHouseDataAdapter = new InHouseDataAdapter())
    using (SqlCommand cmd = new SqlCommand())
    {
        return inHouseDataAdapter.GetDataTable(cmd);
    }
}

...
List<MyObject> listToReturn = new List<MyObject>();
using (DataTable dt = GetDataTableFromInHouseAdapter())
{
    foreach (DataRow dr in dt.Rows)
    {
        listToReturn.Add(new MyObject(dr));
    }
}

This looks more realistic - the method that generates the DataTable belongs in the Data Access Layer, and the transformation to a list of MyObject instances probably belongs in a Facade above the DAL.

In fact I would always consider refactoring a nested using statement into its own method - except when they're closely related (such as SqlConnection/Command, or even InHouseDataAdapter/SqlCommand).

Answer 9

Another possible design would be to close the inHouseDataAdapter or SqlCommand within their "Using" blocks, since a proper IDisposable implementation is required to be tolerant of multiple cleanup attempts. In many situations, I think an explicit close/dispose within a Using block can be a good idea, since an explicitly-called Close method may provide more helpful exceptions than IDisposable.Dispose (the arguments against IDisposable.Dispose throwing an exception don't apply to Close methods).

In this particular scenario, I would affirmatively leave the SqlCommand and inHouseDataAdapter open while copying the DataTable to the List, however. If the GetDataTable returns a DataTable that actually holds data, the foreach loop should execute quickly (so the data-supply entities would not be held open excessively long). Only if it returns a lazy-reading DataTable derivative would the foreach loop take awhile to execute, and in that scenario the data-supply entities would probably need to be held open long enough for the loop to complete.