CODEWITHSUNDEEP
c#loopsif-statement
user935375
user935375

Reputation:

C#: Should I use "if" instead of "while" when a Reader has only one element?

Look at this:

myReader = cmd.ExecuteReader();
if (myReader.HasRows)
{
    while (myReader.Read())
    {
        info = myReader.GetString("info");
        ...
    }
 }
.....

Well, I don´t know why, but when I compile it and use it in another PC, it explodes because of the while. But If I do:

myReader = cmd.ExecuteReader();
if (myReader.HasRows)
{
    if (myReader.Read())    <------------------- NOTICE THE IF
    {
        info = myReader.GetString("info");
        ...
    }
 }
.....

... will work. Notice that I know that I will have only one element every time, because of the logic of the program.

It is OK to do this? It looks horrible and maybe is not a good practice (?). I´m asking for your orientation.

EDIT: Will post some more code, because could help in finding my errors:

 while (myReader.Read())
 {
    info = myReader.GetString("info");
    ...


     /**
      * Write the relevant info in the LOGS
      */
      connection.Close();     <----------   :S  
      connection.Open();      <---------- if I don´t do this I got some problems with the connection!!!!
      string queryLog = "INSERT INTO ....;
      MySqlCommand cmdLog = new MySqlCommand(queryLog, connection);
      cmdLog.ExecuteNonQuery();
 }

Upvotes: 7

Views: 1872

Answers (7)

Adam Houldsworth
Adam Houldsworth

Reputation: 64517

I do this whenever I am expecting just one row, nothing wrong with it.

If I'm expecting a single value, I use ExecuteScalar instead. This will simply return the value held in the first column of the first row of the resultset returned, usally the set only has one column and one row, so you don't tend to notice this.

Also I tend not to bother with if (myReader.HasRows) as if (myReader.Read()) covers this.

I have no idea why the while causes you issues... I'm just offering my opinion on using the if over the while if you aren't expecting multiple rows.

Upvotes: 12

Jon Hanna
Jon Hanna

Reputation: 113332

You can indeed use if, but you also should be able to use while in a case like this without an error. Just changing it to if will work, but if you do the same thing in a case where you can have more than one row, you're going to get the same error. It's important therefore to look at just why the exception was happening.

Let's examine your code:

while (myReader.Read())
 {
    info = myReader.GetString("info");
    ...

So far so good.

     /**
      * Write the relevant info in the LOGS
      */
      connection.Close();     <----------   :S  
      connection.Open();      <---------- if I don´t do this I got some problems with the connection!!!!

Oh uh! You've fixed your "some problems", but did you know what those "some problems" were? It's actually here that you've broken your while.

At the point where I said "so far so good", this is what you have happening:

  1. You have a connection object. It is "open".
  2. You have a data reader object. It is retrieving data from the connection.

Now, by the spec of IDbCommand.ExecuteReader (http://msdn.microsoft.com/en-us/library/68etdec0.aspx), the connection is in use for that reader. You aren't allowed to do anything with that connection except close it, until you call Close() on the reader, either explicitly or through Dispose() as would happen if the reader was assigned in a using block.

Now, strictly this isn't always true. A given implementation is always allowed to give you more than an interface promises (whether according to the interface signature or according to the documentation). Many implementations will "release" the connection for reuse once Read() returns false (the only one I ever implemented did), SQLServer 2005 has a MultipleActiveResultSets=True option that allows the same connection to simultaneously support more than one datareader at once (there are downsides). Still, these loosenings of the rules aside, the only thing we can definitely do with our connection right now, is call Close().

For this reason, when you tried to call cmdLog.ExecuteNonQuery() you got "some problems" in the form of an error message, because you were attempting to use a connection that was in use for something else.

So, you "fixed" this by doing the only thing you could do with the connection - closing it - and then opening it again. For all intents and purposes, you'd a brand new connection!

Now though, when you loop back to the while it calls myReader.Read() again. This method reads from the stream of data coming from the connection and either returns false (if there's no more results) or creates a set of fields based on what it reads from that stream.

But it doesn't, because you closed that connection. So it "explodes" with an exception because you're trying to read from a closed connection and that just can't be done.

Now, when you replaced this while with an if you never came back to the Read() operation you'd busted, so the code works again.

But what are you going to do if you ever need to do this with a resultset that has more than one row?

You could use MARS. See http://msdn.microsoft.com/en-us/library/h32h3abf%28v=vs.80%29.aspx for that. Frankly I'd avoid it unless something gains more than this from it; it has some subtle implications that can be really confusing if you hit them.

You could postpone your next operation until after the readerset is closed. In this case, this would just be a minor saving by taking out the needless Open() and Close():

using(myReader = cmd.ExecuteReader())
{
  while(myReader.Read())
  {
    string info = myReader.GetString("info");
    /* Do stuff */
    using(SqlConnection logConn = new SqlConnection(...))
    {
      logConn.Open();
      string queryLog = "INSERT INTO ....;
      MySqlCommand cmdLog = new MySqlCommand(queryLog, connection);
      cmdLog.ExecuteNonQuery();
    }
  }
}

Now, it may seem wasteful to have two connections rather than one here. However:

  1. Pooling makes creation of connections pretty cheap.
  2. We're releasing it back into the pool as soon as we do the ExecuteNonQuery(), so if there's a lot of this going on across lots of threads the total number of connections on the go will likely be less than twice the number of threads. On the other hand, if there's only one thread doing this, then there's only one extra connection, so who cares.
  3. Closing a connection when it was part-way through providing a datareader may have given it extra clean-up to do before returning to the pool, so you may well be using two connections anyway (I don't know about SQLServer, but I do know that some databases need more work before they can return to the pool in such cases).

In summary, you can't use a connection without reusing it, that's an out-and-out bug. You normally can't have more than one operation on the same connection at a time. Using the MultipleActiveResultSets option with SQLServer2005 lets you do that, though it has complications. Generally, if you want to do more than one DB thing at a time, you should use more than one connection.

All that said, you can still use if rather than while, but do it because it's what makes sense at the time, rather than to fix a bug you don't understand and have it come back some time when if isn't an option.

Upvotes: 2

EWil
EWil

Reputation: 21

Do you dispose your connection and reader properly? Use Using, like:

using (myReader = cmd.ExecuteReader())
{
    if (myReader.HasRows)   
    {   
        while (myReader.Read())   
        {   
            info = myReader.GetString("info");   
            ...   
        }   
    }
}

Upvotes: 1

justin.m.chase
justin.m.chase

Reputation: 13675

I think you should still use the while even if you're expecting only one row then use linq to express the single row expectation when you call it. You can use a coroutine to express this nicely.

public IEnumerable<string> GetInfo() 
{
    // make command and reader...
    while(reader.Read() 
    {
        yield return reader.GetString("info");
    }
 }

Then the caller should use the Single() method to express the expectation of only a single row.

var info = GetInfo().Single();

Upvotes: 0

MethodMan
MethodMan

Reputation: 18863

this code you should add the following to get it to return or not return rows

  myReader = cmd.ExecuteReader();
  myReader.Read();
  if (myReader.HasRows)
  {     
     while (myReader.Read())
     { 
        info = myReader.GetString("info");
     }
  }

Upvotes: -5

Sergey Kalinichenko
Sergey Kalinichenko

Reputation: 726879

I think the use of if makes it clear to the readers of your code that you expect precisely one item. You can add an assert after the end to catch logical errors, like this:

if (myReader.Read())  {
    info = myReader.GetString("info");
    ...
}
Debug.Assert(!myReader.Read(), "Reader has more elements than expected.");

Upvotes: 3

Eric J.
Eric J.

Reputation: 150148

Without using while, you cannot detect an unexpected condition the the reader has more elements than you think it should have. Systems change over time, and assumptions like "the reader will only ever have one element" tend to become incorrect as the system evolves.

If it's important to the function of the system, or of the function of the code you're writing that has that assumption baked in, I would still use a while and include some debug build only code that asserts that you really only received one element.

Upvotes: 2

Related Questions