5
\$\begingroup\$

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()
Dim cnSQL As ADODB.Connection 
Dim sqlCommand As ADODB.Command
Dim prm As ADODB.Parameter


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

Set sqlCommand = New ADODB.Command
sqlCommand.ActiveConnection = cnSQL
sqlCommand.CommandType = adCmdStoredProc
sqlCommand.CommandText = "EstimateInsertUpdate"

Set prm = sqlCommand.CreateParameter("CD", adVarChar, adParamInput, 10)
sqlCommand.Parameters.Append prm
sqlCommand.Parameters("CD").Value = Worksheets("Sheet1").Range("B5")

Set prm = sqlCommand.CreateParameter("Date", adDate, adParamInput)
sqlCommand.Parameters.Append prm
sqlCommand.Parameters("Date").Value = Worksheets("Sheet1").Range("F5")

Set prm = sqlCommand.CreateParameter("Test", adDecimal, adParamInput)
prm.Precision = 4
prm.NumericScale = 4
sqlCommand.Parameters.Append prm
sqlCommand.Parameters("Test").Value = Worksheets("Sheet1").Range("E5")

Set prm = sqlCommand.CreateParameter("EDtm", adDate, adParamInput)
sqlCommand.Parameters.Append prm
sqlCommand.Parameters("EDtm").Value = Worksheets("Sheet1").Range("D5")

Set prm = sqlCommand.CreateParameter("UserID", adVarChar, adParamInput, 10)
sqlCommand.Parameters.Append prm
sqlCommand.Parameters("UserID").Value = Worksheets("Sheet1").Range("G5")

sqlCommand.Execute

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

Set sqlCommand = New ADODB.Command
sqlCommand.ActiveConnection = cnSQL
sqlCommand.CommandType = adCmdStoredProc
sqlCommand.CommandText = "EstimateInsertUpdate"


Set prm = sqlCommand.CreateParameter("CD", adVarChar, adParamInput, 10)
sqlCommand.Parameters.Append prm
sqlCommand.Parameters("CD").Value = Worksheets("Sheet1").Range("B6")

Set prm = sqlCommand.CreateParameter("Date", adDate, adParamInput)
sqlCommand.Parameters.Append prm
sqlCommand.Parameters("Date").Value = Worksheets("Sheet1").Range("F6")

Set prm = sqlCommand.CreateParameter("Test", adDecimal, adParamInput)
prm.Precision = 4
prm.NumericScale = 4
sqlCommand.Parameters.Append prm
sqlCommand.Parameters("Test").Value = Worksheets("Sheet1").Range("E6")

Set prm = sqlCommand.CreateParameter("EDtm", adDate, adParamInput)
sqlCommand.Parameters.Append prm
sqlCommand.Parameters("EDtm").Value = Worksheets("Sheet1").Range("D6")

Set prm = sqlCommand.CreateParameter("UserID", adVarChar, adParamInput, 10)
sqlCommand.Parameters.Append prm
sqlCommand.Parameters("UserID").Value = Worksheets("Sheet1").Range("G6")

sqlCommand.Execute

End Sub
\$\endgroup\$
6
\$\begingroup\$

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.

Instead of this:

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

    Dim result As New ADODB.Parameter
    result.Direction = adParamInput
    result.name = name
    result.value = value

    Select Case VarType(value)
        Case VbVarType.vbBoolean
            result.Type = adBoolean

        Case VbVarType.vbDate
            result.Type = adDate

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

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

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

        Case VbVarType.vbByte, VbVarType.vbInteger, VbVarType.vbLong
            result.Type = adInteger

        Case VbVarType.vbString
            result.Type = adVarChar

        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)
    With New ADODB.Command
        .ActiveConnection = conn
        .CommandType = adCmdStoredProc
        .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

    Dim conn As ADODB.Connection
    Set conn = New ADODB.Connection
    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.

\$\endgroup\$
  • \$\begingroup\$ Not sure I follow your thought process. In your example with runtime error handling, what would go in the ... of Const connString As String = "..." \$\endgroup\$ – mag123 Oct 13 '16 at 18:52
  • \$\begingroup\$ That's your connection string. \$\endgroup\$ – Mathieu Guindon Oct 13 '16 at 18:58
  • \$\begingroup\$ 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? \$\endgroup\$ – mag123 Oct 13 '16 at 19:03
  • \$\begingroup\$ Where? Verify what you're calling and whether you're giving it the correct number of arguments. \$\endgroup\$ – Mathieu Guindon Oct 13 '16 at 19:06
  • \$\begingroup\$ 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? \$\endgroup\$ – mag123 Oct 13 '16 at 19:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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