current community

your communities

more stack exchange communities

Stack Exchange
tour help
careers 2.0
Take the 2-minute tour ×
Stack Overflow is a question and answer site for professional and enthusiast programmers. It's 100% free, no registration required.
up vote 7 down vote favorite

SqlConnection, SqlCommand and SqlDataReader all implement the IDisposable interface. I read about a best practice to always wrap IDisposables into a using block.

So, my common scenario for querying data would look something like this (in greater context of course a mapping tool like linq2sql would be suitable, but lets just assume we want to use this approach here):

using (SqlConnection cn = new SqlConnection("myConnectionstring"))
{
    using (SqlCommand cm = new SqlCommand("myQuery", cn))
    {
        // maybe add sql parameters
        using (SqlDataReader reader = cm.ExecuteReader())
        {
             // read values from reader object
             return myReadValues;
        }
    }
}

Is this the correct way or could that be considered overkill? I am a little unsure about this level of nesting using blocks, but of course i want to do it the correct way. Thanks!

share|improve this question

7 Answers 7

up vote 6 down vote accepted

This is 100% the correct way. If a class leverages IDisposable it should be wrapped in a using statement to ensure that the Dispose() method is called. Further, communicating with an outside technology -unmanaged at that -like SQL Server shouldn't be taken lightly. The SqlCommand object implements IDisposable for a very good reason. The code below is the Dispose() method for the SqlCommand object:

protected override void Dispose(bool disposing)
{
    if (disposing)
    {
        this._cachedMetaData = null;
    }
    base.Dispose(disposing);
}

and as you can see, it's releasing a reference to the _cachedMetaData object so that it too can get cleaned up.

share|improve this answer
    
thanks for your answer, especially for detail about SqlCommands's dispose - exactly the information i was looking for! –  Chips_100 Jun 7 '13 at 14:06
    
@Chips_100, I'm glad I could be of assistance! –  Michael Perrenoud Jun 7 '13 at 14:06
    
Just an FYI, "using" is syntactic sugar for a try --> finally block where the finally calls Dispose. Don't do it that way though - use the using keyword. :) –  Haney♦ Jun 7 '13 at 14:09
    
"it's releasing a reference to the _cachedMetaData object" - but in the OP's example, the _`cachedMetaData` object would become eligible for GC as soon as its containing SqlCommand goes out of scope, which is potentially sooner without the using statement than with it. NB I'm not arguing against calling Dispose: it's probably better not to rely on knowledge of the internals of SqlCommand and just follow the pattern blindly. –  Joe Jun 7 '13 at 14:24
up vote 2 down vote

That is the correct way, if you will be done with the reader. Sometimes, you need the reader to remain open (maybe your method returns it), so it disposing the reader right away would not work. In those cases, there is an overload of the ExecuteReader that can help you:

var cn = new SqlConnection("myConnectionstring");
var cm = new SqlCommand("myQuery", cn);
var reader = cm.ExecuteReader(CommandBehavior.CloseConnection);
return reader;

This will keep the connection and the reader open. Once the reader is closed/disposed, it will also close (and dispose) the connection as well.

using(var reader = GetReader()) //which includes the code above
{
   ...
} // reader is disposed, and so is the connection.
share|improve this answer
    
Your GetReader example is flawed, as it doesn't dispose the connection if new SqlCommand or ExecuteReader throws. See the following answer for a more complete implementation: stackoverflow.com/a/744307/13087 –  Joe Jun 7 '13 at 14:26
    
@Joe Good point, this is not a complete implementation (thanks for the link). The main thing I wanted to point out was the usage of CommandBehavior.CloseConnection when the reader needs to remain open. –  Eren Ersönmez Jun 7 '13 at 15:15
up vote 2 down vote

You can use the following way of typography to get the code closer to the left:

using (SqlConnection cn = new SqlConnection("myConnectionstring"))
using (SqlCommand cm = new SqlCommand("myQuery", cn))
using (SqlDataReader reader = cm.ExecuteReader())
{
     // read values from reader object
     return myReadValues;
}

As other already pointed out, using three nested using blocks is correct though.

share|improve this answer
    
If later on you go auto-identing the rest of your code (i.e.: Ctrl + K + D in Visual Studio), you lose that. –  Renan Jun 7 '13 at 14:17
    
thanks, i didn't know of 'stacking' using declarations before. It's a nice thing to have, alghough i could for example not use that, if i need to add parameters to the SqlCommand. Thanks again for the info! –  Chips_100 Jun 7 '13 at 14:19
1  
@Renan Actually I don't lose it in VS2012. If I remember correctly, VS2010 did support this "using stack" too... –  Matthias Meid Jun 7 '13 at 14:23
2  
I don't think this code works, since the connection is never opened. Stacking it like this looks good, but it's just not viable since there's always stuff that needs to be done (cn.Open(), cm.Parameters.AddWithValue() etc) –  Tobberoth Jul 17 at 9:43
up vote 1 down vote

Yep that's correct. You can miss the braces out in nested using as they are one statement, but I don't feel that adds to readability.

share|improve this answer
up vote 1 down vote

This is not overkill. A using block is good practice because it ensures that the Dispose() method of the object will be called, even if an exception is thrown.

There is a name for this kind of thing, though. It's called code sugar. So:

using (foo bar = new foo()) { //...snip }

Is short hand code for:

foo bar = null;
Exception error = null;
try {
    bar = new foo();
    // ...snip
}
catch (Exception ex) {
    error = ex;
}
finally {
    if (bar != null) bar.Dispose();
    if (error != null) throw error;
}

Either form is equal to the other, they're just different ways to write the same thing. In other words, same difference between for and while: they do basically the same thing but are used in different ways.

using is preferred because it makes the code shorter and more readable, and automates the disposal for you. As to whether you should use it, though, don't listen to people who say you should always do something. It's good practice, granted, but knowing when to use, why to use and the benefits and consequences of using, or not using something is worth way more than doing something because people say you should.

Edit: Eren's answer has an example of a case where you wouldn't want to have a using block for the reader.

share|improve this answer
    
The catch-block is not required if you have a finally-block... –  Alxandr Jun 7 '13 at 14:09
up vote 0 down vote

I don't see any point in disposing the SqlCommand. The SqlConnection and SqlDataReader should be disposed of though.

The SqlConnection because it won't close by itself when it goes out of scope.

The SqlDataReader can keep the SqlConnection busy until the reader is closed.

Not even MSDN examples dispose of the SqlCommand.

share|improve this answer
    
Huh? It implements Idisposable, so it's going to get disposed of. Why would you leave it floating about, until (if) the GC kicks in. –  Tony Hopkinson Jun 7 '13 at 14:05
    
It is widely accepted as a best practice to dispose of any class instance that implements IDisposable. The question isn't Why dispose of the SqlCommand but rather Why Not? –  Evan L Jun 7 '13 at 14:07
    
I'm just pointing out that it is really not more important than getting anything else non-crucial disposed of. @TonyHopkinson, do you run the GC after every object you create goes out of scope? No? Because there is no point. Dispose also uses clock cycles. You should either argue against Garbage Collection in general, or not at all. –  SamiHuutoniemi Jun 7 '13 at 14:14
    
There are classes such as SqlCommand, DataTable that implement IDisposable, but all they do is set a private reference to null, which has no effect if the containing instance is going out of scope anyway. I'd normally dispose SqlCommand, but strictly speaking this answer is correct, omitting the dispose will have no adverse effect. –  Joe Jun 7 '13 at 14:16
1  
Agree with Evan Lewis, and no I don't run GC, except in edge case scenarios. It's far more important to remember to Dispose, than to remember a couple of cases where you could get away with not doing it. Not to mention the inherent scoping it gives you.. –  Tony Hopkinson Jun 7 '13 at 14:37
up vote -1 down vote

I'm not an expert but i know that the using is translated into a try/finally block maybe you can wrap the SqlConnection, SqlCommand and SqlDataReader into a unique try/finally

try {
     // code here with SqlConnection, SqlCommand and SqlDataReader
}
finally
{
  // Dispose call on SqlConnection, SqlCommand and SqlDataReader
 }
share|improve this answer
    
Wasn't me who marked you down, but don't try this. It's not equivalent, and in the event of an exception will leak like a sieve and possibly raise even more exceptions an should raise a pile of warnings from the compiler. –  Tony Hopkinson Jun 7 '13 at 14:03
    
@TonyHopkinson I answered just to have a feedback. Can you explain further why it's not equivalent? thank you very much! –  Davide Lettieri Jun 7 '13 at 14:07
1  
If sql connection connect raises an exception. Finally executes and then you get a null reference exception on the sqlcommand. To get round that you have to test for nil. –  Tony Hopkinson Jun 7 '13 at 14:09

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.