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.
