Is there a reason to check for null inside multiple using clausule in c#?

Question

Is there a reason to check for null in the last using? Seems to me like it's most likely not gonna be needed?

using (var connection = new SqlConnection(connectionString))
{
using (var command = new SqlCommand(commandString, connection))
{
    using (var reader = command.ExecuteReader())
    {
      if (reader != null) {
         // Use the reader
      }
    }
   }
}

EDIT:

Should i close reader.Close() and connection.Close() inside the using if i use it like that:

            using (var varConnection = Locale.sqlConnectOneTime(Locale.sqlDataConnectionDetailsDZP))
            using (var sqlWrite = new SqlCommand(preparedCommand, varConnection)) {
                 while (sqlWrite.Read()) {
                  //something to do.
                 }
                 sqlWrite.Close();
                 varConnection.Close();
            }




    public static SqlConnection sqlConnectOneTime(string varSqlConnectionDetails) {
        SqlConnection sqlConnection = new SqlConnection(varSqlConnectionDetails);
        sqlConnect(sqlConnection);
        if (sqlConnection.State == ConnectionState.Open) {
            return sqlConnection;
        }
        return null;
    }

Is using of close necessary in following example or i can skip those two? sqlWrite.Close(); varConnection.Close();

With regards,

MadBoy

Just throwing this out there: you only need one set of braces here, which might make the code easier to read. eg: using(...) using(...) using(...) { ... } — Alex Lyman, Feb 08 '10 at 09:01
Are you specifically asking about `ExecuteReader()` and `using` clauses, or are you asking about `using` clauses in general. Many of the responders are answering the former. — Robert Paulson, Feb 08 '10 at 09:09
Yes but if i need to add parameters command.Parameters.AddWithValue("@varRachunek", varRachunek); then i need to use the braces. — MadBoy, Feb 08 '10 at 09:15
I'm asking about this typical example. Using ExecuteReader and using. — MadBoy, Feb 08 '10 at 09:17

H H · Accepted Answer · 2010-02-08T09:07:02.177

4

No, this is not necessary.

But that follows from the definition of ExecuteReader, it is not related to the using clause. ExecuteReader either returns a (non-null) DataReader object or it throws an exception.
The expresion in the if statement will always be true (if it is reached).

And by leaving out the if and all the superfluous brace-pairs you can make this much easier to read:

using (var connection = new SqlConnection(connectionString))
using (var command = new SqlCommand(commandString, connection))
using (var reader = command.ExecuteReader())
{
    // Use the reader
}

edited Feb 08 '10 at 09:07

answered Feb 08 '10 at 08:59

H H

263,252
30
330
514

That's the assumption but since it is possible for the reader to be null why avoid the check? – Cory Charlton Feb 08 '10 at 09:04
@Cory, how can the reader be null? That's not how I read the specs. – H H Feb 08 '10 at 09:08
See my answer as well. Just because ExecuteReader() is supposed to return a non-null SqlDataReader doesn't mean it always will. – Cory Charlton Feb 08 '10 at 09:12
So theoretically i should check each var for being null ? connection, command and reader. instead of new SqlConnection i use my own function to connect to sql public static SqlConnection sqlConnectOneTime(string varSqlConnectionDetails) { SqlConnection sqlConnection = new SqlConnection(varSqlConnectionDetails); sqlConnect(sqlConnection); if (sqlConnection.State == ConnectionState.Open) { return sqlConnection; } return null; } which returns null, maybe i should change it to return something else instead? – MadBoy Feb 08 '10 at 09:23
5

@Madboy: No you shouldn't. Your code will become a mess. I still don't buy Cory's idea that ExecuteReader could return null. But if it does your code _should_ fail with a null-ref exception. – H H Feb 08 '10 at 09:27
@Henk, Which part of the specs are you refering to when you say your reading indicates it can't be null? I'm looking at the help for SqlCommand and finding it inconclusive. Personally I still do the null check so that ReSharper doesn't complain. – Mike Two Feb 08 '10 at 09:58
Nice to see the removal un unnecessary braces suggested, makes the code easier to work with. – stevehipwell Feb 08 '10 at 10:19
@Mike: you can infer this from the fact that there is no mention of when null is returned. – H H Feb 08 '10 at 10:54
@Henk, I see your point. I'm not sure if I feel comfortable infering that. I guess it is really just a personal level of paranoia issue. Unfortunately there is no way for the API to clearly specifiy that it won't return a null. I am also still stuck with trying to keep ReSharper quiet in this case. I guess I could add an annotation to make it happy. – Mike Two Feb 08 '10 at 11:09
Mike, if you apply the same rule to the Connection and Command objects you get 3 nested usings interleaved with 3 if/else statements, and no real improvement in code quality. – H H Feb 08 '10 at 12:50
@Henk, not quite true. You're constructing connection and command. ExecuteReader is the only method call. I do feel confident that the constructor will not return null, but a method call is a different story. – Mike Two Feb 08 '10 at 13:20
Mike, you're right. But my point remains: You should not check every time a method returns a reference. Only when it is an expected case. – H H Feb 08 '10 at 13:42
Guys, can you please also let me know if it's necessary to close sql connection, sql query inside using ? varConnection.Close(); reader.Close(); before closing bracket of using? – MadBoy Feb 09 '10 at 10:53
using (var varConnection = Locale.sqlConnectOneTime(Locale.sqlDataConnectionDetailsDZP)) using (var sqlWrite = new SqlCommand(preparedCommand, varConnection)) { } } should i close varConnection and sqlWrite or i can leave it be? – MadBoy Feb 09 '10 at 10:55
@madboy, that has been asked and answered a lot here: No, `using` ensures Dispose and Dispose calls Close (or equiv). – H H Feb 09 '10 at 21:45

score 3 · Answer 2 · answered Feb 08 '10 at 08:58

3

No. Why would you do that? The documentation doesn't suggest that it might be null. What would you do if it was null, anyway? Try it again?

answered Feb 08 '10 at 08:58

mqp

70,359
14
95
123

Grzenio · Answer 3 · 2010-02-08T09:43:33.263

1

the using statements are not going to check for null for you, so if command.ExecuteReader() can return null you have to check for it explicitly, just like in your code snippet.

EDIT: Looks like in this particular case ExecuteReader() should now return null, so you can avoid it. Please remember that you need it in general case though.

edited Feb 08 '10 at 09:43

answered Feb 08 '10 at 08:59

Grzenio

35,875
47
158
240

@mquander in this instance perhaps it's not necessary, but in the general case, `using` clauses do not guarantee that the reference is not null, just that the reference has `IDisposable.Dispose()` called at the end of the scope of the using block. – Robert Paulson Feb 08 '10 at 09:03
I guess that's true, if that's really what the poster was asking about. – mqp Feb 08 '10 at 09:07
I'm especially interested in using Using and SQL not overall approach for checking if object can be null. Resharper tends to suggest that i should check for null but if it can't be null then i can just skip that step. – MadBoy Feb 08 '10 at 09:13
@MadBoy: Resharper suggests you check because a SqlDataReader can be null. Regardless of what the ExecuteReader() documentation says it is a possibility and I'm not sure what you're hoping to get by not checking it? – Cory Charlton Feb 08 '10 at 09:17
Heh :-) You're not already doing while(reader.Read()) ? .. while(reader != null && reader.Read()) ;-) – Cory Charlton Feb 08 '10 at 09:24
I wasn't but it's a good idea, but it never happened for me that SqlDataReader was null. It always threw exception of some kind if there was an error and never reached the if null check. – MadBoy Feb 08 '10 at 09:43
@mquander, thanks for pointing this out, I updated my reply accordingly. – Grzenio Feb 08 '10 at 09:44

score 1 · Answer 4 · edited May 23 '17 at 12:24

1

Seems like the null check is worth it simply to avoid that slim chance that the object is null. Take a look at my answer to How can I modify a queue collection in a loop?

In the example Queue.Dequeue() should never return a null but it does. It's an extreme case but I don't see why you wouldn't want to avoid an unhandled exception, especially if it's as easy as if (object != null)

For Henk (you can run the code yourself and get similar results):

alt text http://www.ccswe.com/temp/Untitled.png

Either way I was simply stating my opinion that just because the documentation says it will do one thing doesn't mean it always will. Not sure why I got the downvote simply because someone has a different opinion :-)

Edit: Stupid tool tip, either way you can see the processed is 9998065 and the null values encountered is 2264. If there is something fundamentally wrong with my example code I'd be interested in hearing it. I'm gonna back away from this thread now.

edited May 23 '17 at 12:24

Community

1
1

answered Feb 08 '10 at 09:03

Cory Charlton

8,868
4
48
68

1

I'm kind of skeptical about Dequeue returning nulls, but even if it does that cannot be extended to ExecuteReader. – H H Feb 08 '10 at 09:29
I post a full example code that shows it both returning a null value AND decreasing the count. Granted the documentation on Queue mentions thread safety but Dequeue() also says it returns the item, which it doesn't always. – Cory Charlton Feb 08 '10 at 09:35
Cory, you are still confusing Queue and DataReader. – H H Feb 08 '10 at 10:33
And I took a look at that other question. You are simply demonstrating a race condition while using a Queue without locking. There is nothing wrong with DeQueue() itself. – H H Feb 08 '10 at 10:42

score 1 · Answer 5 · edited May 23 '17 at 12:01

Since you are specifically using SqlConnection and SqlCommand - no, there is no reason for this check.

However, the ADO interfaces are so that you can plug in any other DB provider and use them via the ADO base classes (DbConnection, DbCommand etc). It seems that some providers are returning null sometimes (MySQL maybe?), which may be why the programmer inserted this. Or just because ReSharper issues a warning here? These may be reasons for this, even though it is not necessary if the providers used follow the contract.

score 0 · Answer 6 · answered Feb 08 '10 at 10:03

In this case you should not check reader for null.

But you may use Code Contracts library and use Contract.Assume (or Contract.Assert) to write your assumptions about code. In this case you can use tools for static analysis and easily remove this checks from your release builds.

Is there a reason to check for null inside multiple using clausule in c#?

6 Answers6

Linked