0

i have a button that when clicked inserts data from textbox and combobox fields into database tables, but every time i insert it gives me "Invalid attempt to call read when reader is closed". How can i get rid of this error. And tips on optimising the code are welcome, because i know im a total noob. thanks

private void btnSave_Click(object sender, RoutedEventArgs e)
    {
        try
        {
            SqlConnection sqlCon = new SqlConnection(@"Data Source=(localdb)\mssqllocaldb; Initial Catalog=Storagedb;");
            sqlCon.Open();
            string Query1 = "insert into location(Storage, Shelf, columns, rows) values(" + txtWarehouse.Text + ", " + txtShelf.Text + ", " + txtColumn.Text + ", " + txtRow.Text + ")";
            SqlCommand sqlCmd = new SqlCommand(Query1, sqlCon);
            SqlDataAdapter dataAdp = new SqlDataAdapter(sqlCmd);
            dataAdp.SelectCommand.ExecuteNonQuery();
            sqlCon.Close();

        }
        catch (Exception er)
        {
            MessageBox.Show(er.Message);
        }
        try
        {
            SqlConnection sqlCon = new SqlConnection(@"Data Source=(localdb)\mssqllocaldb; Initial Catalog=Storagedb;");
            sqlCon.Open();
            string Query3 = "SELECT LOCATION_ID FROM LOCATION WHERE storage='" + txtWarehouse.Text + "' AND shelf='" + txtShelf.Text + "' AND columns='"
                + txtColumn.Text + "' AND rows='" + txtRow.Text + "'";
            SqlCommand sqlCmd1 = new SqlCommand(Query3, sqlCon);
            SqlDataReader dr = sqlCmd1.ExecuteReader(); ;
            while (dr.Read())
            {
                string LocationId = dr[0].ToString();
                dr.Close();
                string Query2 = "insert into product(SKU, nimetus, minimum, maximum, quantity,location_ID,category_ID,OrderMail_ID) values ('" + txtSku.Text + "','" + txtNimetus.Text + "', '"
                + txtMin.Text + "', '" + txtMax.Text + "', '" + txtQuan.Text + "', '" + LocationId + "', '" + (cbCat.SelectedIndex+1) + "', '" + (cbMail.SelectedIndex+1) + "')";
                SqlCommand sqlCmd = new SqlCommand(Query2, sqlCon);
                SqlDataAdapter dataAdp = new SqlDataAdapter(sqlCmd);
                dataAdp.SelectCommand.ExecuteNonQuery();
            }
            sqlCon.Close();
        }
        catch (Exception ed)
        {
            MessageBox.Show(ed.Message);
        }
    }
5
  • The DataAdapter is not needed here. Just call ExecuteNonQuery from the SqlCommand that you build.
    – Steve
    Nov 20, 2017 at 18:56
  • remove dr.Close(); line
    – Steve
    Nov 20, 2017 at 18:58
  • "There is already an open datareader associated with this command which must be closed first" is the error i get now and no data is now inserted into database
    – kiq
    Nov 20, 2017 at 19:04
  • 1
    You should consider changing how you handled your SQL query. As you wrote it, it's extremely vulnerable to SQL injection and anyone who uses your application could drop your entire database on you with a script entered into a text box. Check out Paul's answer here to get an idea on how to use Database Parameters to prevent SQL injection. He also provides a good link to help you better understand SQL injection. Nov 20, 2017 at 19:06
  • you are trying to read in a while loop but second line in your while loop you are calling dr.Close() try using the debugger and also start code reviewing your own code.. I would also recommend separating the Selects and Updates into there own smaller methods to make your code more readable.. it's kind of messy and read up on SQL Injection as well
    – MethodMan
    Nov 20, 2017 at 19:34

1 Answer 1

2

Let's try to make some adjustments to your code.

  • First thing to consider is to use a parameterized query and not a string concatenation when you build an sql command. This is mandatory to avoid parsing errors and Sql Injections
  • Second, you should encapsulate the disposable objects in a using statement to be sure they receive the proper disposal when you have finished to use them.
  • Third, you can get the LOCATION_ID from your table without running a separate query simply adding SELECT SCOPE_IDENTITY() as second batch to your first command. (This works only if you have declared the LOCATION_ID field in the first table as an IDENTITY column)
  • Fourth, you put everything in a transaction to avoid problems in case some of the code fails unexpectedly

So:

SqlTransaction tr = null;
try
{
    string cmdText = @"insert into location(Storage, Shelf, columns, rows) 
                       values(@storage,@shelf,@columns,@rows);
                       select scope_identity()"; 
    using(SqlConnection sqlCon = new SqlConnection(.....))
    using(SqlCommand cmd = new SqlCommand(cmdText, sqlCon))
    {
         sqlCon.Open();
         using( tr = sqlCon.BeginTransaction())
         {
             // Prepare all the parameters required by the command
             cmd.Parameters.Add("@storage", SqlDbType.Int).Value = Convert.ToInt32(txtWarehouse.Text);
             cmd.Parameters.Add("@shelf", SqlDbType.Int).Value = Convert.ToInt32(txtShelf.Text);
             cmd.Parameters.Add("@columns", SqlDbType.Int).Value = Convert.ToInt32(txtColumn.Text );
             cmd.Parameters.Add("@rows", SqlDbType.Int).Value = Convert.ToInt32(txtRow.Text);

             // Execute the command and get back the result of SCOPE_IDENTITY
             int newLocation = Convert.ToInt32(cmd.ExecuteScalar());

             // Set the second command text
             cmdText = @"insert into product(SKU, nimetus, minimum, maximum, quantity,location_ID,category_ID,OrderMail_ID) 
                         values (@sku, @nimetus,@min,@max,@qty,@locid,@catid,@ordid)";

              // Build a new command with the second text
              using(SqlCommand cmd1 = new SqlCommand(cmdText, sqlCon))
              {
                  // Inform the new command we are inside a transaction
                  cmd1.Transaction = tr;

                  // Add all the required parameters for the second command
                  cmd1.Parameters.Add("@sku", SqlDbType.NVarChar).Value = txtSku.Text;
                  cmd1.Parameters.Add("@nimetus",SqlDbType.NVarChar).Value = txtNimetus.Text;
                  cmd1.Parameters.Add("@locid", SqlDbType.Int).Value = newLocation;
                  .... and so on for the other parameters required

                  cmd1.ExecuteNonQuery();
                  // If we reach this point the everything is allright and
                  // we can commit the two inserts together
                  tr.Commit();
             }
        }
    }
}
catch (Exception er)
{
    // In case of exceptions do not insert anything...
    if(tr != null) 
       tr.Rollback();
    MessageBox.Show(er.Message);
}

Notice that in the first command I use parameters of type SqlDbType.Int because you haven't used single quotes around your text. This should be verified against the real data type of your table columns and adjusted to match the type. This is true as well for the second command where you put everything as text albeit some of those fields seems to be integer (_location_id_ is probably an integer). Please verify against your table.

3
  • thank you, but when i finish the code i get "Cannot assign to 'cmd' because it is a 'using variable" at the line where building a new command with the second text
    – kiq
    Nov 20, 2017 at 20:12
  • and when i rename the 'cmd' to 'SqlCommand cmd1' and add all required parameters to 'cmd1' i get "ExecuteScalar requires the command to have a transaction when the connection assigned to the command is in a pending local transaction. The Transaction property of the command has not been initialized"
    – kiq
    Nov 20, 2017 at 20:22
  • Yes, you are correct about the first problem. We need to build a new SqlCommand and inform it that we are inside a transaction. Answer updated
    – Steve
    Nov 20, 2017 at 20:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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