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

Are you using MS-Access as database backend? – Steve Jan 25 '14 at 19:51 — Steve, Jan 25 '14 at 19:51
Steve I am using Access 2010. Thanks – user1532468 Jan 26 '14 at 11:15 — user1532468, Jan 26 '14 at 11:15

score 2 · Accepted Answer · edited May 23 '17 at 11:49

2

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

edited May 23 '17 at 11:49

Community

1
1

answered Jan 25 '14 at 19:42

Steve

213,761
22
232
286

Steve in your code, how would I apply transactions to it. Thanks – user1532468 Jan 26 '14 at 11:45
This line 'Dim rowAffected oledbCmd.ExecuteNonQuery()' produces error of: 'End of statement expected'. How do I correct. Thanks – user1532468 Jan 26 '14 at 11:50
Missing the equal sign, fixing now – Steve Jan 26 '14 at 12:58
Added code for Transaction, but keep in mind that I have not tested it with Access 2010. – Steve Jan 26 '14 at 13:09
Steve thanks for example of transaction. However, in your code example, I am getting an error of: 'Data type mismatch in criteria expression.' and am confused because in my original code, it inserted fine. Do i have to amend your code somehow because as far as I can see, all the fields are correct. Thanks – user1532468 Jan 26 '14 at 17:52
In addition to that, I am also getting errors of: 'OleDbTransaction is a type and cannot be used as an expression'. Thanks – user1532468 Jan 26 '14 at 17:56
Not sure what could be the problem because I can't see the database fields type. In the original code everything is treated as strings and so the underlying table fields should be of text type. If this is not true then some value need to be converted (Like the date columns for example) – Steve Jan 26 '14 at 17:56
For the OleDbTransaction, it is the declaration as C# fixing now – Steve Jan 26 '14 at 17:59
The database fields "[Date-time received]" and "[Date-time due]" are defined as DateTIme or Texts? – Steve Jan 26 '14 at 18:02
As far as the data type mismatch, the field 'Quantity' is a number. Do i need to convert is your code. Thanks – user1532468 Jan 26 '14 at 18:02
Yes add a Convert.ToInt32(....) (if integer) or use the appropriate Convert.ToXXXXX – Steve Jan 26 '14 at 18:04
Steve when I debug I can see in auto window that all fields have the correct data, but it still throws a data type error. Have I converted correctly. Dim Quantity As Integer = Convert.ToInt32(txtBoxQuantity.Text) – user1532468 Jan 26 '14 at 18:14
let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/46117/discussion-between-steve-and-user1532468) – Steve Jan 26 '14 at 18:44
Steve all good now. Thanks very much for your help and your patience in solving my problem. All the best. – user1532468 Jan 27 '14 at 11:37

score 2 · Answer 2 · answered Jan 25 '14 at 19:49

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

answered Jan 25 '14 at 19:49

Damir Arh

17,637
2
45
83

As a new user to VB.NET, I am not familiar with Transactions. Do you have a tutorial link or docs I can view. Thanks – user1532468 Jan 26 '14 at 11:16
@user1532468 Transaction are a database concept, not a programming language concept. [This Wikipedia entry](http://en.wikipedia.org/wiki/Database_transaction) should get you started. – Damir Arh Jan 26 '14 at 19:49

Is this coding correct for inserting into 2 tables

2 Answers2