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
That whole block is never going to execute. You just declared
rs and haven't assigned it yet: it's always going to be
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.
Set rs = Nothing is potentially extending the lifetime of the
rs object: without it, there's no in-scope reference to
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
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 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
Worksheets collection, by its name, when you already have a reference to it?
Now, handle runtime errors:
Public Sub DoSomething()
On Error GoTo CleanFail
'error handling/recovering code
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
rngToPrint - a better name would be simply
FileName - no prefix? Parameter isn't used, remove it.
Whenever you feel the need to comment "this chunk of code does X", like here:
...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
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
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
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.