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.
