Making repeated ADODB queries from Excel-SQL Server

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

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

StackExchange Code Review Q#143895, answer score: 8