Passing multiple parameters to an SQL query

I am trying to use this VBA code to pass a SQL stored procedure multiple values from an excel sheet.

In order to have the procedure run multiple times and insert multiple sets of information into the database, I copied and pasted the parameter section of the code each time I wanted to insert new data and hard coded which excel cell to pull data from.

Is there a way to consolidate the code so that I can reference cells A1:A10 for parameter 1, cells B1:B10 for parameter 2, cells C1:C10 for parameter 3 and so on without copying the code over again?

Sub Module2()

cnSQL.Open "Provider=SQLOLEDB.1; uid=test; Pwd=test; Initial Catalog = test; Data source=test"

sqlCommand.ActiveConnection = cnSQL
sqlCommand.CommandText = "EstimateInsertUpdate"

sqlCommand.Parameters.Append prm
sqlCommand.Parameters("CD").Value = Worksheets("Sheet1").Range("B5")

sqlCommand.Parameters.Append prm
sqlCommand.Parameters("Date").Value = Worksheets("Sheet1").Range("F5")

prm.Precision = 4
prm.NumericScale = 4
sqlCommand.Parameters.Append prm
sqlCommand.Parameters("Test").Value = Worksheets("Sheet1").Range("E5")

sqlCommand.Parameters.Append prm
sqlCommand.Parameters("EDtm").Value = Worksheets("Sheet1").Range("D5")

sqlCommand.Parameters.Append prm
sqlCommand.Parameters("UserID").Value = Worksheets("Sheet1").Range("G5")

sqlCommand.Execute

cnSQL.Open "Provider=SQLOLEDB.1; uid=test; Pwd=test; Initial Catalog = PMW; Data source=test"

sqlCommand.ActiveConnection = cnSQL
sqlCommand.CommandText = "EstimateInsertUpdate"

sqlCommand.Parameters.Append prm
sqlCommand.Parameters("CD").Value = Worksheets("Sheet1").Range("B6")

sqlCommand.Parameters.Append prm
sqlCommand.Parameters("Date").Value = Worksheets("Sheet1").Range("F6")

prm.Precision = 4
prm.NumericScale = 4
sqlCommand.Parameters.Append prm
sqlCommand.Parameters("Test").Value = Worksheets("Sheet1").Range("E6")

sqlCommand.Parameters.Append prm
sqlCommand.Parameters("EDtm").Value = Worksheets("Sheet1").Range("D6")

sqlCommand.Parameters.Append prm
sqlCommand.Parameters("UserID").Value = Worksheets("Sheet1").Range("G6")

sqlCommand.Execute

End Sub


Sub Module2 is a terrible name to use for a procedure. Not only it doesn't say anything about what it does, it looks like the [default] name of some standard code module.

Name things, it's important. Procedures do something, their names should start with a verb, and you should be able to read it and know roughly what it does just by the name.

Consider:

Public Sub InsertOrUpdateEstimates()


Also indent your code - it's harder to read code that begins in column 1 no matter what.

Sub DoSomething()
DoSomeThings
DoSomeMoreThings
End Sub


Consider:

Sub DoSomething()
DoSomeThings
DoSomeMoreThings
End Sub


You're defining 5 parameters per command, but declare only 1 ADODB.Parameter object variable; this changes the meaning of the variable as the procedure executes, and this makes for bug-prone and confusing code. If a variable stands for "the UserID parameter", then it's reasonable to expect that variable to mean the same thing throughout the scope of the procedure. Same for the connection objects.

The problem is exactly that: the scope of the procedure is too wide - it's doing too many things.

You've essentially written a script - it goes top to bottom and executes an ordered sequence of operations and then completes.

You don't need to "consolidate" the code, you need to tear it apart and extract each responsibility into its own function, break it down into multiple, smaller, more specialized procedures.

You want to run a command for each row of a given range: you need a procedure that runs a command for a single row, and call it from another procedure.

The command takes 5 parameters; make the procedure receive 5 values as ...parameters:

Private Sub InsertOrUpdateEstimate(ByVal CD as String, ByVal theDate As Date, ByVal test As Single, ByVal EDtm As Date, ByVal UserID As String)


The body of that procedure will need to create an ADODB.Parameter object for each parameter value: instead of repeating the code for that 5 times, make another function that's responsible for creating an ADODB parameter - something like this:

Private Function CreateCommandParameter(ByVal name As String, ByVal value As Variant, Optional ByVal numPrecision As Integer = 4, Optional ByVal numScale As Integer = 4) As ADODB.Parameter

result.name = name
result.value = value

Select Case VarType(value)
Case VbVarType.vbBoolean

Case VbVarType.vbDate

Case VbVarType.vbCurrency
result.Precision = numPrecision
result.NumericScale = numScale

Case VbVarType.vbDouble
result.Precision = numPrecision
result.NumericScale = numScale

Case VbVarType.vbSingle
result.Precision = numPrecision
result.NumericScale = numScale

Case VbVarType.vbByte, VbVarType.vbInteger, VbVarType.vbLong

Case VbVarType.vbString

Case Else
Err.Raise 5, Description:="Data type not supported"
End Select

Set CreateCommandParameter = result
End Function


Now, InsertOrUpdateEstimate can look like this:

Private Sub InsertOrUpdateEstimate(ByVal conn As ADODB.Connection, ByVal CD As String, ByVal theDate As Date, ByVal test As Single, ByVal EDtm As Date, ByVal UserID As String)
.ActiveConnection = conn
.CommandText = "EstimateInsertUpdate"
.Parameters.Append CreateCommandParameter("CD", CD)
.Parameters.Append CreateCommandParameter("Date", theDate)
.Parameters.Append CreateCommandParameter("Test", test)
.Parameters.Append CreateCommandParameter("EDtm", EDtm)
.Parameters.Append CreateCommandParameter("UserID", UserID)
.Execute
End With
End Sub


Notice it's receiving the ADODB.Connection as a parameter, so it's responsible for executing a command, but not for setting up the connection.

So all that's left to do is to call that procedure inside the body of some loop that iterates all the rows we want to look at.

But there's more to consider: what if an error occurs because some of the data in row 7 is of the wrong type? What if the connection times out?

It would be a good idea to initiate a transaction before we start looping, and only commit the transaction once all rows have been correctly processed - and if any error happens, we can rollback the entire transaction, fix things and start over.

Think of a transaction as a bank transaction: you're moving dollars from one account to the other - if something wrong happens in the middle of the transaction, you don't want the money to be withdrawn from the source account but not deposited into the target account! A transaction either runs completely, or not at all: wrapping all these insert/update commands in a transaction is probably a good idea.

To do this correctly you need to handle runtime errors. Consider this procedure:

Public Sub InsertOrUpdateEstimates()
Const connString As String = "..."
Const fromRow As Long = 1
Const toRow As Long = 10

Dim CD As String
Dim theDate As Date
Dim test As Single
Dim EDtm As Date
Dim UserID As String

On Error GoTo CleanFail

conn.Open connString

conn.BeginTrans

Dim currentRow As Long
With Sheet1
For currentRow = fromRow To toRow
CD = .Cells(currentRow, 2).value
EDtm = .Cells(currentRow, 4).value
test = .Cells(currentRow, 5).value
theDate = .Cells(currentRow, 6).value
UserID = .Cells(currentRow, 7).value
InsertOrUpdateEstimate conn, CD, theDate, test, EDtm, UserID
Next
End With

conn.CommitTrans

CleanExit:
conn.Close
Exit Sub

CleanFail:
conn.RollbackTrans
MsgBox Err.Description & vbNewLine & "Transaction was rolled back, no changes were made", vbExclamation
Resume CleanExit

End Sub


Notice the Sheet1 object is being referred to directly instead of being fetched from the Worksheets collection everytime it's needed; the With block removes all redundancies here. Also we're reading the values using the Cells function instead of Range, so we can easily supply a row/column number to pick up a single value.

You can push the segregation of responsibilities to the next level by moving the whole ADODB-handling code to its own class - see materializing any ADODB query and creating ADODB parameters on the fly for more information; the For loop above could look something like this (and that replaces the helper CreateCommandParameter function and CreateCommandParameter procedure, too):

Const sql As String = "exec dbo.EstimateInsertUpdate ?,?,?,?,?;"
conn.BeginTrans

With Sheet1
For currentRow = fromRow To toRow
SqlCommand.ExecuteNonQuery conn, sql, _
.Cells(currentRow,2).value, _
.Cells(currentRow,4).value, _
.Cells(currentRow,5).value, _
.Cells(currentRow,6).value, _
.Cells(currentRow,7).value
Next
End With

conn.CommitTrans


The fewer things a procedure does, the clearer the code becomes; there shouldn't ever be a reason to copy+paste code - if you find yourself pressing Ctrl+C, ask yourself "is there not a better way?" before you press Ctrl+V.

• Not sure I follow your thought process. In your example with runtime error handling, what would go in the ... of Const connString As String = "..." – mag123 Oct 13 '16 at 18:52
• That's your connection string. – Mathieu Guindon Oct 13 '16 at 18:58
• Got it. When trying to run that code i am getting a compile error saying 'wrong number of arguments or invalid property assignment.' Any idea what could be causing that? – mag123 Oct 13 '16 at 19:03
• Where? Verify what you're calling and whether you're giving it the correct number of arguments. – Mathieu Guindon Oct 13 '16 at 19:06
• I don't understand how you've broken up the code. Do I need each section of code that you wrote or are some sections substitutes for other sections? – mag123 Oct 13 '16 at 19:13