How to use SqlDataReader in a loop?

Question

I need to process the data from the list using SqlDataReader. To do this, I wrote a for loop in which the data for the query will be supplied and get into SqlDataReader. But after the first iteration, the loop breaks. An error is displayed that you need to close SqlDataReader.

List<string> days = new List<string>() { "Monday", "Tuesday", "Wednesday", "Thursday", "Friday" };

SqlConnection cnn = new SqlConnection(@"Con");
cnn.Open();

SqlCommand cmd = cnn.CreateCommand();
List<string> data = new List<string>();

for (int i = 0; i < days.Count;i++)
{
    cmd.CommandText = $@"select * from Table where Day = '{days[i]}'";
    
    SqlDataReader reader = cmd.ExecuteReader();
    
    while (reader.Read())
    {
        data.Add(reader[0].ToString());
    }
}

Here, for example, I use the "days" list, but in the program itself the list is obtained from a query, so it can be larger. This is where the error occurs. I tried to close SqlDataReader, but the problem is that it cannot be opened back. In any case, I need to somehow get data from SqlDataReader in a loop.

**Always use parameterized sql and avoid string concatenation** to add values to sql statements. This mitigates SQL Injection vulnerabilities and ensures values are passed to the statement correctly. See [How can I add user-supplied input to an SQL statement?](https://stackoverflow.com/q/35163361/1260204), [Exploits of a Mom](https://xkcd.com/327/), and [Why do we always prefer using parameters in SQL statements?](https://stackoverflow.com/q/7505808/1260204). — Igor, Apr 02 '23 at 13:31
Even when the parameter values are known it is a good habit to use parameters instead. — Igor, Apr 02 '23 at 13:32
Implement a class whose properties map to the table columns and then try opening and closing your connection inside the loop — Son of Man, Apr 02 '23 at 13:35
The main idea - query in a loop - is suspicious. SQL query is expencive: we should pass query via net (time), RDBM must parse it (time), optimize (time), validate credential, execute (and may be read from HDD), return results... If yo want to *rob a bank*, do it once and take a *million*, and stop keep doing it whenever you need a tenner for a glass of beer. Here we can easly change loop into a single `in` — Dmitry Bychenko, Apr 02 '23 at 13:41

Dmitry Bychenko · Answer 1 · 2023-04-02T13:32:28.577

Yes, you should Dispose instances that implement IDisposable: a quick patch, but not the best soultion is to add using:

...

for (int i = 0; i < days.Count; i++) {
  cmd.CommandText = $@"select * from Table where Day = '{days[i]}'";
    
  // using to Dispose reader
  using (SqlDataReader reader = cmd.ExecuteReader()) {
    while (reader.Read())
      data.Add(reader[0].ToString());
  }
}

A better aproach is to call query just once (SQL is expensive):

var data = new List<string>();

var days = new List<string>() { 
  "Monday", "Tuesday", "Wednesday", "Thursday", "Friday" 
};

using SqlConnection cnn = new SqlConnection(@"Con");

cnn.Open();

//TODO: change * into the only field(s) you want to fetch
// Since days are list of constants I haven't created bind variables:
//   - No sql injection
//   - Constants are specific and in specific order (workdays) so
//     I want the query be specific and not mixed with in (:param1, ... :paramN)
//     for arbitrary days 
string sql = 
  $@"select *
       from Table
      where day in ({string.Join(", ", days.Select(day => $"'{day}'"))})";

using SqlCommand cmd = cnn.CreateCommand(sql, cnn);

using SqlDataReader reader = cmd.ExecuteReader();

while (reader.Read()) {
  data.Add(Convert.ToString(reader[0]));
}

How to use SqlDataReader in a loop?

1 Answers1