Multiple ExecuteReader optimization

Question

Is there any way to optimize code below. I'm using 3 executeReader for different results

SqlCommand command = new SqlCommand("select DeliveryID,Name from deliveryphone WHERE PhoneNumber= '" + textBox1.Text + "'", con);
        SqlDataReader read = command.ExecuteReader();

        while (read.Read())
        {

            SqlDeliveryID = (read["DeliveryID"].ToString());
            textBox2.Text = (read["Name"].ToString());

        }
        read.Close();
        SqlCommand command2 = new SqlCommand("select  Adress from DeliveryAdress WHERE DeliveryID= '" + SqlDeliveryID + "' ", con);
        SqlDataReader read2 = command2.ExecuteReader();

        while (read2.Read())
        {
            comboBox1.Items.Add(read2["Adress"].ToString());

        }
        read2.Close();
        SqlCommand command3 = new SqlCommand("select top 1 Adress,Location,Floor,Comments from DeliveryAdress WHERE DeliveryID= '" + SqlDeliveryID + "' order by DefaultAdress desc", con);
        SqlDataReader read3 = command3.ExecuteReader();

        while (read3.Read())
        {
            comboBox1.Text = (read3["Adress"].ToString());
            textBox3.Text = (read3["Location"].ToString());
            comboBox2.Text = (read3["Floor"].ToString());
            textBox5.Text = (read3["Comments"].ToString());

        }

Is there any way to combine this 3 reader into 1?

If your code works but you need suggestions how to optimize it, please move your question to the [Code Review](https://codereview.stackexchange.com/) site. — dymanoid, May 04 '18 at 15:24
1) Google [sql how to join tables](https://www.google.com/search?q=sql+how+to+join+tables). That will help you understand how to create a single query to get all the data you need. 2) **Always use parameters for your values!** See [How can I add user-supplied input to an SQL statement?](https://stackoverflow.com/q/35163361/1260204) on how you can parameterize your query. If you do not think this is important, what would happen if someone entered the text value `'; DROP TABLE deliveryphone` in `textBox1.Text`. — Igor, May 04 '18 at 15:32
Firstly you're creating a giant security hole...use a stored procedure instead. Second re-write the sql as a join so you don't have to deal with multiple records sets. — BillRuhl, May 04 '18 at 15:34
@BillRuhl Stored procs aren't necessary. He needs to use parametrized queries. — , May 04 '18 at 16:14
@amy I didn't say that stored procs are necessary and yes using a stored procedure is more secure and a parametrized query. — BillRuhl, May 04 '18 at 16:53
A stored procedure is not "more secure" than a parametrized query. Where did you get that idea — , May 04 '18 at 16:53
This is because when you pass a parameter to a stored procedure the parameter is it's treated as a literal. Any dynamically generated sql is vulnerable to truncation. But the parameters passed to a stored procedure will throw an exception if truncated. Now this is all assuming that the procedure does not accept unfiltered input ;) https://learn.microsoft.com/en-us/sql/relational-databases/security/sql-injection?view=sql-server-2017 — BillRuhl, May 04 '18 at 17:15

score 0 · Answer 1 · answered May 04 '18 at 15:24

0

You might want to use multiple result sets with a single reader.

How to read multiple resultset from SqlDataReader?

answered May 04 '18 at 15:24

tomasat

578
8
11

score 0 · Answer 2 · answered May 04 '18 at 15:48

Google sql how to join tables, that will help you understand how to create a single query to get all the data you need.
Always use parameters for your values! See How can I add user-supplied input to an SQL statement? on how you can parameterize your query. If you do not think this is important, what would happen if someone entered the text value '; DROP TABLE deliveryphone in textBox1.Text.
Wrap your disposable types in using blocks so they are closed/disposed when they fall out of scope.

const string query = @"SELECT dp.DeliveryID, dp.Name, da.Adress, da.Location, da.Floor, da.Comments
    FROM DeliveryPhone dp INNER JOIN DeliveryAdress da ON dp.DeliveryID = da.DeliveryID
    WHERE dp.PhoneNumber=@phoneNumber";


using(SqlCommand command = new SqlCommand(query, con))
{
  // I guessed on the sql type and length
  command.Parameters.Add(new SqlParameter("@phoneNumber", SqlDbType.VarChar, 50) {Value = textBox1.Text});
  con.Open(); // is the connection always open? Really you should create connections on an as needed basis and then dispose of them
  using(SqlDataReader read = command.ExecuteReader())
  {
    if(reader.Read())
    {
      textBox2.Text = read.GetString(1);
      comboBox1.Text = read.GetString(2);
      textBox3.Text = read.GetString(3);
      comboBox2.Text = read.GetString(4);
      textBox5.Text = read.GetString(5);
      // if you need the other values you can get those as well
    }
  }
}

Multiple ExecuteReader optimization

2 Answers2