How fast should I close the block in use?

Question

During the code review the other day, the question arose of how to quickly close the use block. One camp said: "Once you are done with the object"; another, "someday before he goes out of scope."

In this particular example, there is a DataTable and a SqlCommand that should be deleted. We need to refer to one operator as well, and we need to iterate over 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 of SqlCommand as soon as you finish using it. Do not run potentially lengthy operations, such as iterating a table, inside another object using a 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, no matter what, and none of them is resource-intensive, so it is not important to dispose immediately.

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

Edit:. Several people have indicated that the DataTable does not need to be deleted (see Corey Sunwold’s answer ), and that the first Camp 1 example is uglier than it should be. Here are a few revised examples that also take into account the fact that most of the time I have to set some properties in SqlCommand. If anyone has seen or can come up with a better example to support any 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 would agree that the readability argument is now significantly reduced and that this is not the best example of what I'm trying to ask. (This is especially true as soon as I tell you that SqlConnection is closed before the GetDataTable () method completes, and there are no measurable performance differences for the data used in this instance.) If I can add to my question it's late when there is any difference, do I delete the object immediately? For example, as Gregory Higley mentioned , a shared resource such as an OS descriptor.

Edit: (explaining my choice of answer) Many thanks to everyone who contributed to our opinions, examples, and other useful feedback! It seems that we split evenly, but what stands out among everyone is the idea that "Camp 1 is definitely right, but depending on the facility, Camp 2 may be in order." I meant that this is a general discussion about the disposal of all types of objects, but I chose a bad example to illustrate this. Since most of the discussion focused on this particular example, I chose an answer that gave me important information about the specific objects used, and proved that I need to carefully consider each object when making such a decision. (In any case, it would be difficult to choose the "best answer" to a question as vague as in my title). Future readers with the same dilemma, please see all the answers below, as many of them raise interesting points.

Answer 1

Basically, I agree with Camp 1. However, you should note it, probably not needed to host a DataTable .

Answer 2

I think it depends on what unmanaged resources are held by a one-time object. In general, although I am with you in Camp 2.

If an unmanaged resource is somehow shared, especially if it is a handle to an operating system of some type, I will try to get rid of it as quickly as possible.

Answer 3

I'm in Camp 2, just like you. Sometimes the criticality of resources is determined by the available resources on the machine. The Camp 2 solution believes in a preventative measure where objects are removed as you did, not lazy camp 1.

Answer 4

Usually we go to camp 2 - for the reason that the external using statement uses the SqlTransaction object.

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

Answer 5

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

In some rare cases, I would prefer Camp1 if there was a clear need to manage the resource very quickly. I mean, if you run into some problems linking the connection a bit later. But in most cases there would be no penalty if you go Camp2.

Answer 6

It all comes down to how long you consider it wise to stick to an external resource. Data binding isnt free, so it makes sense to clear it as soon as possible. For readability, I would still maintain my position, since it is very clear that it stores resource objects here.

But actually this is a bit of a clean question, since we are talking about low milliseconds here. I would say that you are mistaken on the side of camp 1, especially if you have used the data requested for a long time.

Also, I would get rid of cleaning up the DataTable. It is not required for a managed entity without resource references. And at the same time quite a lot denies the argument of readability.

Answer 7

+1 for camp 2.

In the sample of your camp 1 there are objects that can be used AFTER WITHOUT DISPOSAL. I think this is not a problem with your exact scenario, but may be a problem with other things. The Camp 2 version forces you to place objects in correctly nested areas, making the code safe.

Example:

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

Answer 8

An example of your camp 1 is a little straw man, he should not be as ugly as this.

If performance was a problem (which is probably rare and unlikely in this far-fetched example), I would go for a cleaner version of Camp 1, refactoring the DataTable to 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)); } } 

It looks more realistic - the method that the DataTable generates belongs to the data access layer, and the conversion to the list of MyObject instances probably refers to the facade over the DAL.

In fact, I would always consider refactoring a nested using statement in my own method - unless they are closely related (e.g. SqlConnection / Command or even InHouseDataAdapter / SqlCommand).

Answer 9

Another possible design is to close inHouseDataAdapter or SqlCommand in their Use blocks, as the correct implementation of IDisposable is required in order to be tolerant of several cleanup attempts. In many situations, I think that explicitly closing / disposing in the Use block may be a good idea, since an explicit Close method may provide more useful exceptions than IDisposable. Dispose (arguments against IDisposable.Dispose throwing exception do not apply to Close methods).

In this particular scenario, I would affirmatively leave SqlCommand and inHouseDataAdapter open when copying a DataTable to a list. If GetDataTable returns a DataTable that actually stores the data, the foreach loop should execute quickly (therefore, data transfer objects will not remain open too long). Only if it returns a DataTable derivative with lazy reading will the foreach loop take some time, in which case the data transfer objects should probably be open long enough for the loop to complete.