3
\$\begingroup\$

I need to run 12 queries on a SQL Server DB. Currently I pass the "SELECT..." string to a function below.

Is it optimal to open and close the connection again and again (as my code is doing), or can I set up an ADODB connection as a global variable and reuse the same connection?

Sub ConnectServer(FileName As String, rngToPrint As Range, strSqlQuery As String)
    Dim conn As ADODB.Connection
    Dim rs As ADODB.Recordset
    Dim sConnString As String
    Dim iCols As Long

    If Not (rs Is Nothing) Then
        If (rs.State And adStateOpen) = adStateOpen Then rs.Close
        Set rs = Nothing
    End If

    sConnString = "Poop;"

    Set conn = New ADODB.Connection
    Set rs = New ADODB.Recordset

    conn.CommandTimeout = 90

    conn.Open sConnString

    Set rs = conn.Execute(strSqlQuery)

    'Add Headers
    For iCols = 0 To rs.Fields.Count - 1
        Sheets(rngToPrint.Worksheet.Name).Cells(rngToPrint.Row - 1, iCols + 1).Value = rs.Fields(iCols).Name
    Next

    If Not rs.EOF Then
        ' Transfer result.
        rngToPrint.CopyFromRecordset rs
    ' Close the recordset
        rs.Close
    Else
        MsgBox "Error: No records returned.", vbCritical
    End If

    If CBool(conn.State And adStateOpen) Then conn.Close
    Set conn = Nothing
    Set rs = Nothing
End Sub
\$\endgroup\$
  • 1
    \$\begingroup\$ this may be worth reading... \$\endgroup\$ – sous2817 Oct 11 '16 at 15:11
  • 1
    \$\begingroup\$ ^^ that. note that ADODB does connection pooling, too. \$\endgroup\$ – Mathieu Guindon Oct 11 '16 at 15:24
7
\$\begingroup\$

can I set up an ADODB connection as a global variable and reuse the same connection?

That would be the single worst thing to do. First because you would be making a critical resource globally scoped, and therefore you would lose control over its lifetime.

Second, because ADODB does connection pooling for you anyway, so the "cost" of opening a connection isn't really a thing; if a connection exists in the connection pool and is free to use, that's the connection ADODB will give you - it's there already.

From code perspective, a database connection should always be as short-lived as possible: open, fetch/execute, close.


This is interesting:

Dim rs As ADODB.Recordset

If Not (rs Is Nothing) Then
    If (rs.State And adStateOpen) = adStateOpen Then rs.Close
    Set rs = Nothing
End If

That whole block is never going to execute. You just declared rs and haven't assigned it yet: it's always going to be Nothing.

If CBool(conn.State And adStateOpen) Then conn.Close
Set conn = Nothing
Set rs = Nothing

If no error occurred, your connection is always going to be opened here. And if an error occurred, your code isn't even running anymore. Make the conn.Close unconditional, and get rid of the redundant assignments to Nothing; the objects are going out of scope at End Sub, they're being destroyed anyway.

Actually, that Set rs = Nothing is potentially extending the lifetime of the rs object: without it, there's no in-scope reference to rs beyond rs.Close, and (assuming the VBA runtime can destroy objects while inside a procedure scope) VBA can destroy the object. But because of that Set rs = Nothing, you're keeping a reference around, and VBA cannot destroy the object until it passes that instruction.


If Not (rs Is Nothing) Then

The parentheses are redundant here: If Not rs Is Nothing evaluates to the same thing. Not (rs Is Nothing) simply forces rs Is Nothing to be evaluated to a value before Not processes (inverts) it, ...but that's exactly what happens either way, because of operator precedence rules.

If CBool(conn.State And adStateOpen) Then

The explicit conversion to Boolean is redundant here. That And is actually a bitwise operator here; it's not clear how the bitwise operation correctly converts to a Boolean - I'd suggest having a little helper function for this:

Private Function HasFlag(ByVal value As Long, ByVal flag As Long) As Boolean
    HasFlag = (value And flag) = flag
End Function

That way you can do If HasFlag(conn.State, adStateOpen) Then and have crystal-clear intentions.


I said earlier:

And if an error occurred, your code isn't even running anymore.

That's quite a problem. You need to handle runtime errors. Database connections drop, a syntax error in strSqlQuery is likely (you're taking it as a parameter - who knows if it's legal SQL?), the rngToPrint could be in another workbook - so many things can go wrong!

First thing to fix is this:

Sheets(rngToPrint.Worksheet.Name).Cells(...)

The Sheets collection contains charts and other non-worksheet objects. Use the Worksheets collection when you want worksheets. But the biggest issue is that it's implicitly referring to the ActiveWorkbook. Yet nothing says rngToPrint comes from that workbook: if the active workbook has worksheets "Sheet1", "Sheet2" and "Sheet3" and rngToPrint is in another workbook and its Worksheet.Name is "DATA", your code blows up right here.

Why retrieve the worksheet from the Sheets/Worksheets collection, by its name, when you already have a reference to it?

rngToPrint.Worksheet.Cells(...)

Now, handle runtime errors:

Public Sub DoSomething()
    On Error GoTo CleanFail

    'code

CleanExit:
    'cleanup code
    Exit Sub
CleanFail:
    'error handling/recovering code
    Resume CleanExit
End If

With this pattern you close the recordset and connection under the CleanExit label, and because it runs whether or not an error has occurred then you make the clean-up conditional to the state of your object references (e.g. if rs Is Nothing don't try to close it) and/or your connection.


Your parameters are all passed ByRef implicitly, and they could all passed ByVal instead. The name ConnectServer doesn't convey what the procedure is doing at all (it's doing much more than just connecting to a server), and it's implicitly Public as well; making it explicitly Public would be good, or make it Private if it's not meant to be used beyond the scope of the module it's written in.

Hungarian Notation prefixes are superfluous, and misleading:

  • iCols - you're looking at a Long, not an "Integer".
  • sConnString - the name says "string", what's the s for?
  • rngToPrint - a better name would be simply destination.
  • FileName - no prefix? Parameter isn't used, remove it.

Whenever you feel the need to comment "this chunk of code does X", like here:

'Add headers

...you're looking a a chunk of code that actually belongs elsewhere. In this case, iCols belongs in the scope of a helper procedure that iterates a given recordset's fields, and writes the names of the fields in the first row of a given Range - add an explicit reference to the ADOR (Microsoft ActiveX Data Objects Recordset 6.0 Library) library too, so you can early-bind the Fields and Field object types:

Private Sub WriteColumnHeader(ByVal target As Worksheet, ByVal source As ADOR.Fields, Optional ByVal headerRow As Long = 1)
    Dim current As Long
    Dim fld As ADOR.Field
    For Each fld In source
        current = current + 1
        target.Cells(headerRow, current).Value = fld.Name
    Next
End Sub

Note that iterating a collection of objects is faster with a For Each loop; keep For loops for iterating arrays.

I find it a bit weird that you're writing the column names in row rngToPrint.Row - 1: that means I can't pass "A1" to your function as a destination, because you'll be attempting to write to row 0 and blow up. I would assume the first row of the destination range is the row where the column headers belong.


MsgBox "Error: No records returned.", vbCritical

"no records" isn't necessarily an error. It depends on the query! If I give your procedure a query that lists all reports for employee injuries in the past week, I'll be happy to see no records returned.

I wouldn't bother interpreting the results like this in this procedure - it's beyond its purpose: you don't know whether the client code is expecting records or not.

You could, however, be informative about it:

MsgBox "Query was executed, but no records were returned.", vbInformation

And keep vbCritical for actual errors, like if there's a syntax error in the query, or if the connection times out or otherwise can't be made to the server for whatever reason.

\$\endgroup\$
  • 1
    \$\begingroup\$ That first sentence is ruthless ;) \$\endgroup\$ – Raystafarian Oct 11 '16 at 18:49
  • \$\begingroup\$ Haha I feel this is more of a roast than a review... Seriously though- thanks, I never expected such a detailed response \$\endgroup\$ – User632716 Oct 12 '16 at 8:42

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.