Is this coding correct for inserting into 2 tables

Question

I have some code that inserts data into 2 tables in an access db. Looking at my code, I figured there may be a better way to code this and being a new user to VB.NET, would appreciate some help. The code I am using is working ok, but is it correct.

I am aware of sql injection problems with this code and will change to params for production.

Comments would be great as I am still learning. Many thanks

For i = 0 To lvSelectedItems.Items.Count - 1

                box = lvSelectedItems.Items.Item(i).Text
                custref = lvSelectedItems.Items.Item(i).SubItems.Item(1).Text

                sql = "Insert into Requests ([Request no], Customer, Dept, [Type], [Service level], " &
                      "[Date-time received], [Received by], [Date-time due], Quantity, [Cust requestor], Status) " &
                      "Values ('" & itm & "', '" & cmbCustomer.Text & "', '" & cmbDept.Text & "', 'B', '" & rbServiceLevel.ToString & "', " &
                      "'" & dtpDateReceived.Value & "', '" & txtBy.Text & "', '" & dtpDateDue.Value & "', '" & txtBoxQuantity.Text & "', '" & cmbRequestBy.Text & "', 'O')"

                oledbCmd.CommandText = sql
                oledbCmd.Connection = oledbCnn
                dr = oledbCmd.ExecuteReader()
                dr.Close()

                sql = "Insert into [Request Boxes] ([Request no], Customer, Box) " &
                "Values ('" & itm2 & "', '" & cmbCustomer.Text & "', '" & box.ToString & "')"

                oledbCmd.CommandText = sql
                oledbCmd.Connection = oledbCnn
                dr = oledbCmd.ExecuteReader()

            Next
            MessageBox.Show("You have successfully completed the activity", "Box return successfull")
            Close()

CODE UPDATE

Try
                ' Here the connection should be already open
                ' VB.NET will insist on inserting the parenthesis in the code below.
                ' which is different to your code example.

                OleDbTransaction(tran = oledbCnn.BeginTransaction())
                oledbCmd.Transaction = tran

                For i = 0 To lvSelectedItems.Items.Count - 1
                    box = lvSelectedItems.Items.Item(i).Text
                    custref = lvSelectedItems.Items.Item(i).SubItems.Item(1).Text

                    sql = "Insert into Requests ([Request no], Customer, Dept, [Type], [Service level], " &
                          "[Date-time received], [Received by], [Date-time due], Quantity, [Cust requestor], " &
                          "Status) Values (?, ?, ?, 'B', ?, ?, ?, ?, ?, ?, '0')"

                    oledbCmd.Parameters.Clear()
                    oledbCmd.CommandText = sql
                    oledbCmd.Connection = oledbCnn
                    oledbCmd.Parameters.AddWithValue("@p1", itm)
                    oledbCmd.Parameters.AddWithValue("@p2", cmbCustomer.Text)
                    oledbCmd.Parameters.AddWithValue("@p3", cmbDept.Text)
                    oledbCmd.Parameters.AddWithValue("@p4", rbServiceLevel.ToString)
                    oledbCmd.Parameters.AddWithValue("@p5", dtpDateReceived.Value)
                    oledbCmd.Parameters.AddWithValue("@p6", txtBy.Text)
                    oledbCmd.Parameters.AddWithValue("@p7", dtpDateDue.Value)
                    oledbCmd.Parameters.AddWithValue("@p8", txtBoxQuantity.Text)
                    oledbCmd.Parameters.AddWithValue("@p9", cmbRequestBy.Text)
                    Dim rowsAffected = oledbCmd.ExecuteNonQuery()
                    If rowsAffected = 0 Then
                        ' Fail to insert. Display a message and rollback everything
                        tran.Rollback()
                        Return
                    End If
                    sql = "Insert into [Request Boxes] ([Request no], Customer, Box) " &
                           "Values (?,?,?)"
                    oledbCmd.CommandText = sql
                    oledbCmd.Parameters.Clear()
                    oledbCmd.Parameters.AddWithValue("@p1", itm2)
                    oledbCmd.Parameters.AddWithValue("@p2", cmbCustomer.Text)
                    oledbCmd.Parameters.AddWithValue("@p3", box.ToString)
                    rowsAffected = oledbCmd.ExecuteNonQuery()
                    If rowsAffected = 0 Then
                        ' Fail to insert. Display a Message and rollback everything
                        tran.Rollback()
                        Return
                    End If
                Next
                ' if we reach this point, then all the commands have been 
                ' completed correctly we could commit everything
                tran.Commit()

            Catch ex As Exception
                Console.WriteLine(ex.Message)
                tran.Rollback()
            End Try

Answer 1

Your code doesn't seem to be incorrect, but contains a very bad practice and an improper use of ExecuteReader.

First, do no use string concatenation to build a sql command. This leaves the door open to sql injection security and parsing problems due to characters like a single quote present in the user input. This problem should be avoided using a parameterized query.

Second. ExecuteReader is for reading and should not be used to insert data. While ExecuteReader works also with INSERT queries, it is better to use ExecuteNonQuery to know if your insert has been succesful

' Here the connection should be already open
Dim tran as OleDbTransaction = oledbCnn.BeginTransaction()

Try
    oledbCmd.Transaction = tran

    For i = 0 To lvSelectedItems.Items.Count - 1
        box = lvSelectedItems.Items.Item(i).Text
        custref = lvSelectedItems.Items.Item(i).SubItems.Item(1).Text

        sql = "Insert into Requests ([Request no], Customer, Dept, [Type], [Service level], " &
              "[Date-time received], [Received by], [Date-time due], Quantity, [Cust requestor], " & 
              "Status) Values (?, ?, ?, 'B', ?, ?, ?, ?, ?, ?, '0')"

         oledbCmd.Parameters.Clear()
         oledbCmd.CommandText = sql
         oledbCmd.Connection = oledbCnn
         oledbCmd.Parameters.AddWithValue("@p1",  itm)
         oledbCmd.Parameters.AddWithValue("@p2",  cmbCustomer.Text )
         oledbCmd.Parameters.AddWithValue("@p3",  cmbDept.Text)
         oledbCmd.Parameters.AddWithValue("@p4",  rbServiceLevel.ToString)
         oledbCmd.Parameters.AddWithValue("@p5",  dtpDateReceived.Value)
         oledbCmd.Parameters.AddWithValue("@p6",  txtBy.Text)
         oledbCmd.Parameters.AddWithValue("@p7",  dtpDateDue.Value)
         oledbCmd.Parameters.AddWithValue("@p8",  txtBoxQuantity.Text)
         oledbCmd.Parameters.AddWithValue("@p9",  cmbRequestBy.Text )
         Dim rowsAffected = oledbCmd.ExecuteNonQuery()
         if rowsAffected = 0 Then
             ' Fail to insert. Display a message and rollback everything
             tran.Rollback()
             return
         End If
         sql = "Insert into [Request Boxes] ([Request no], Customer, Box) " &
                "Values (?,?,?)"
         oledbCmd.CommandText = sql
         oledbCmd.Parameters.Clear()
         oledbCmd.Parameters.AddWithValue("@p1",  itm2)
         oledbCmd.Parameters.AddWithValue("@p2",  cmbCustomer.Text )
         oledbCmd.Parameters.AddWithValue("@p3",  box.ToString)
         rowsAffected = oledbCmd.ExecuteNonQuery()
         if rowsAffected = 0 Then
             ' Fail to insert. Display a Message and rollback everything
             tran.Rollback()
             return
         End If
    Next
    ' if we reach this point, then all the commands have been 
    ' completed correctly we could commit everything
    tran.Commit()

Catch ex As Exception
    Console.WriteLine(ex.Message)
    tran.Rollback()
End Try 

Answer 2

Apart from the string concatenation issue for which you already said that you are aware of, I would suggest the following changes:

1. Use oledbCmd.ExecuteNonQuery() instead of oledbCmd.ExecuteReader().

oledbCmd.CommandText = sql
oledbCmd.Connection = oledbCnn
oledbCmd.ExecuteNonQuery()

2. Make use of transactions to maintain database consistency, i.e. to either succesfully perform all inserts or none.

oleDbTran = oledbCnn.BeginTransaction()
Try
    For i = 0 To lvSelectedItems.Items.Count - 1
        ' loop with inserts
    Next
    oleDbTran.Commit()
Catch
    oleDbTran.Rollback()
End Try