7
\$\begingroup\$

I do a lot of copying excel vba code related to classic ADO from workbook to workbook, so I've decided to add the code to a Class Module in a single utility workbook and then in all other books just reference the utility book and use instances of the ADO wrapper class.

The class module is called clDataAccess

I would like to know any obvious flaws in the coding:

Option Explicit

'::::::::::::::::::::
'::
'::  Class Variable Declarations
'::
'::::::::::::::::::::

Private mcnConnectString As String
Private mcnConnect As ADODB.Connection


'::::::::::::::::::::
'::
'::  on initialization open an ADO connection through to the server
'::
'::::::::::::::::::::
Private Sub Class_Initialize()

    mcnConnectString = _
        "PROVIDER=SQLOLEDB.1;" & _
        "P******D=*********;" & _
        "PERSIST SE**RITY INFO=****;" & _
        "USER ID=**********;" & _
        "INITIAL CATALOG=*******;" & _
        "DATA SOURCE=*****;" & _
        "USE PROCEDURE FOR PREPARE=*;" & _
        "AUTO TRANSLATE=****;" & _
        "CONNECT TIMEOUT=*;" & _
        "COMMAND TIMEMOUT=*" & _
        "PACKET SIZE=****;" & _
        "TAG WITH COLUMN COLLATION WHEN POSSIBLE=****"

    Call ensureClosedConn(mcnConnectString)

End Sub


'::::::::::::::::::::
'::
'::  connection string property
'::  user could need to change the connection
'::
'::::::::::::::::::::
Property Let connectionString(ByVal stConn As String)
    mcnConnectString = connectionString
End Property


':::::::::::::::::
'::
':: This procedure attempts to create a connection to
':: the database using the specified connection string.
'::
'::::::::::::::::::::
Private Function ensureClosedConn(ByVal sConnection As String) As Boolean

On Error GoTo ErrorHandler

    If (mcnConnect Is Nothing) Then
        Set mcnConnect = New ADODB.Connection
        With mcnConnect
            .connectionString = sConnection
            .CommandTimeout = 0
            .Open
            .Close
        End With
    Else
        If (mcnConnect.State And 1) = 1 Then mcnConnect.Close
    End If

Exit Function
ErrorHandler:
    Err.Raise 99999, , "Error occured inside routine ensureClosedConn of clDataAccess"
End Function

':::::::::::::::::
'::
':: Client may wish to receive the recordset
'::
'::::::::::::::::::::   
Public Function cGetRecordset(ByVal sqlString As String) As ADODB.Recordset

    Call ensureClosed_PubConn
    mcnConnect.Open
    Set fRecordset = New ADODB.Recordset

    With fRecordset
        .Open _
            Source:=sqlString, _
            ActiveConnection:=mcnConnect, _
            CursorType:=adOpenForwardOnly, _
            LockType:=adLockReadOnly, _
            Options:=adCmdText
    End With

    Set cGetRecordset = fRecordset
End Function


':::::::::::::::::
'::
':: This procedure attempts to move some data
'::
'::::::::::::::::::::
Public Sub updateWorkSheet( _
    ByVal sqlString As String, _
    ByRef rngDest As Excel.Range _
    )

On Error GoTo ErrorHandler

    Call ensureClosed_PubConn
    mcnConnect.Open
    Dim r As ADODB.Recordset
    Set r = New ADODB.Recordset

    With r
        ':: Extract and copy the required records.
        .Open _
            Source:=sqlString, _
            ActiveConnection:=mcnConnect, _
            CursorType:=adOpenForwardOnly, _
            LockType:=adLockReadOnly, _
            Options:=adCmdText
        rngDest.CopyFromRecordset r
        .Close
    End With

    ':: drop the recordset and its connection
    If Not (r Is Nothing) Then
        If (r.State And 1) = 1 Then r.Close
        Set r.ActiveConnection = Nothing
        Set r = Nothing
    End If

ErrorHandler:
    Err.Raise 99999, , "Error occured inside routine updateWorkSheet of clDataAccess"
End Sub    'updateWorkSheet
\$\endgroup\$
0
7
\$\begingroup\$

Don't do this:

'::::::::::::::::::::
'::
'::  Class Variable Declarations
'::
'::::::::::::::::::::

Fields appear in the declarations section of a class, you can't really miss them. Banner comments like this are a distraction more than anything else. At least you didn't make the :::::::::::::: banner stretch all the way around the comment as a fancy border.


Here are Rubberduck 2.0 code inspection results (you're lucky, v2.0 isn't released yet.. but I could get your code to parse as-is on my build, so here it goes):

Code Quality Issues

  • The return value for ensureClosedConn is never assigned: this means your function will only ever return False, the default value for a Boolean.
  • Parameter stConn is not used: this means the connectionString property isn't assigned the value that the client code passed to the property... which is rather disturbing and surprising.
  • connectionString is a write-only property: that's a design smell - if you can set a property value, you should be able to also read it. read-only properties are fine, write-only properties aren't.
  • Parameter rngDest can be passed by value: you're not re-assigning the object reference there, there's not really a need to pass the parameter ByRef.

Maintainability & Readability Issues

  • Consider renaming variable r: always prefer meaningful names. "r" isn't particularly meaningful; ADODB.Recordset objects are typically called rs, which would be a slightly better name because it would be less surprising.
  • Member connectionString is implicitly public: you've given every member an explicit access modifier - that's great! This one is implicitly public though. Oversight? Better make it explicitly public.

Language Opportunities

  • Use of obsolete Call statement: Call is completely unnecessary, it's only in the language to support older, wrinkled, ancestral, dinosaurian versions of VB. You can safely remove it. When passing parameters, remove the braces:

     ensureClosedConn mcnConnectString
    

I would remove the Class_Initialize handler, and get the connection string out of that class - you have a setter (Property Let) procedure for the connectionString property for a reason: at one point or another you thought it would be a good idea that the client code decides where it wants to connect to - and that was a great idea. Let go of responsibilities, focus on what's essential.

I don't understand ensureClosedConn at all. Why open a new connection only to close it? There's no need for this method IMO. And calling it from the class' initialize handler is a double-no-no: you're making this code...

Set foo = New clDataAccess

...take much longer to execute than one would expect, especially if that client code is hoping to assign a connection string and then get a recordset.


Nitpicks

Consider sticking to convention and using PascalCase for public member names; you'll present an API that's consistent with everything else in the language.

This also means to name a class DataAccess if you mean to call it DataAccess - there's no need for it to have a cl prefix to mean "hey look that's a class module!", classes get their own little icon in IntelliSense, and their own little folder in the IDE's Project Explorer anyway.


Error Handling

Your error handling isn't at the right level of abstraction.

ErrorHandler:
    Err.Raise 99999, , "Error occured inside routine ensureClosedConn of clDataAccess"

Imagine client code that calls updateWorksheet, and there's a syntax error in the SQL statement they're giving you. The error the client code will receive will simply say:

"Error occured inside routine updateWorkSheet of clDataAccess"

No idea if that's because the query timed out, or because it returned so many rows that Excel ran out, or whatever other reason the thing could ever fail for.

Let these errors bubble up, let go of that responsibility, let the caller blow up if they don't handle errors - they'll know how to handle runtime errors better than the code at that level of abstraction. The good thing is that you're not displaying a message box. The bad thing is that the client code can't know what to display in a message box, because you've stripped all useful error information.

\$\endgroup\$
9
  • \$\begingroup\$ The banner formatting was lifted from some Rob Bovey code - but I guess it is a bit distracting but more importantly not saying anything more than is plainly obvious - I will minimize the banners. I'm just working through all other suggestions - thanks for taking time to analyse my code so fully - appreciated! \$\endgroup\$
    – whytheq
    Jan 9 '16 at 19:03
  • \$\begingroup\$ write-only is rare but valid sometimes surely? Because of the information within connectionString I think it's ok to be write-only? If it gets set twice to the same value, because a client does not know it's value, who cares - I'm just concerned about nosey objects reading it. \$\endgroup\$
    – whytheq
    Jan 9 '16 at 19:18
  • 1
    \$\begingroup\$ your Rubberduck comments are excellent - how active is the poject? Do you have many contributors? \$\endgroup\$
    – whytheq
    Jan 9 '16 at 19:21
  • \$\begingroup\$ I've deleted the initialize event procedure - maybe the addition of a terminate event procedure would be polite to ensure all database objects are cleared away? \$\endgroup\$
    – whytheq
    Jan 9 '16 at 19:41
  • \$\begingroup\$ (last comment/question for now - promise!) In terms of letting the error bubble up, in order to achieve this should I just delete the On Error...etc statements in my code? \$\endgroup\$
    – whytheq
    Jan 9 '16 at 19:44
7
\$\begingroup\$

Suggestions:

  1. Don't assume the cursor type of the recordset, the user might want a different type.

  2. Consider making a ConnectionString class that lets the user use properties to get and let the individual parameters, but allows assigning or returning a complete string

  3. Don't force the connection to close, as the user might intend to make subsequent calls.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the suggestions. re. Point 1: I'm specifically using this class within excel development - are various cursor types possible in this context? Point 2: Great idea. \$\endgroup\$
    – whytheq
    Jan 9 '16 at 18:34
2
\$\begingroup\$

After first refactor I've now got the following - it is quite a bit simpler and several responsibilities are now moved to the client or subsequent classes. I've deleted quite a few on error goto statements to allow errors to bubble up. Also included a terminate event procedure just to tidy up any connection objects.

Private mcnConnectString As String
Private mcnConnect As ADODB.Connection

Private Sub Class_Terminate()
    On Error Resume Next

    If Not (mcnConnect Is Nothing) Then
        If (mcnConnect.State And 1) = 1 Then mcnConnect.Close
        Set mcnConnect = Nothing
    End If
End Sub


''::  connection string property - publically it is write-only
''::
Public Property Let ConnectionString(ByVal stConn As String)
    mcnConnectString = stConn
End Property
Private Property Get ConnectionString() As String
    ConnectionString = mcnConnectString
End Property


':: This procedure attempts to create a connection to
':: the database using a specified connection string.
'::
Private Sub EnsureOpenConn(ByVal sConnection As String)

    If (mcnConnect Is Nothing) Then
        Set mcnConnect = New ADODB.Connection
        With mcnConnect
            .ConnectionString = sConnection
            .CommandTimeout = 0
            .Open
        End With
    End If

    ':: check if closed and needs opening
    If Not (mcnConnect.State And 1) = 1 Then
        mcnConnect.Open
    End If

End Sub


':: Following is included as caller may with to receive
':: the filled recordset to use later
'::
Public Function GetRecordset(ByVal sqlString As String) As ADODB.Recordset

    EnsureOpenConn Me.ConnectionString

    Dim rs As ADODB.Recordset
    Set rs = New ADODB.Recordset
    With rs
        .Open _
            Source:=sqlString, _
            ActiveConnection:=mcnConnect, _
            CursorType:=adOpenForwardOnly, _
            LockType:=adLockReadOnly, _
            Options:=adCmdText
    End With

    Set GetRecordset = rs

End Function


':: This procedure attempts to move some data into a specified range
'::
Public Sub UpdateSheet( _
    ByVal sqlString As String, _
    ByVal rngDest As Excel.Range _
    )

    EnsureOpenConn Me.ConnectionString

    Dim rs As ADODB.Recordset
    Set rs = New ADODB.Recordset
    With rs
        .Open _
            Source:=sqlString, _
            ActiveConnection:=mcnConnect, _
            CursorType:=adOpenForwardOnly, _
            LockType:=adLockReadOnly, _
            Options:=adCmdText 
        rngDest.CopyFromRecordset rs
        .Close
    End With

    ':: drop the recordset 
    If Not (rs Is Nothing) Then
        If (rs.State And 1) = 1 Then rs.Close
        Set rs.ActiveConnection = Nothing
        Set rs = Nothing
    End If

End Sub
\$\endgroup\$
4
  • \$\begingroup\$ Feel free to ask a new question if you want a follow-up post, just link back to this question for context :-) \$\endgroup\$ Jan 9 '16 at 21:30
  • \$\begingroup\$ @Mat'sMug Hi Mat - I was questioning myself as to why I opened and then closed a connection & felt there was a definite reason. Page 632 of 2nd Edition of "Professional Excel Development" talks about ADO connection pooling and mentions that even once the connection is closed OLE DB holds an open connection in the background. \$\endgroup\$
    – whytheq
    Jan 11 '16 at 12:28
  • \$\begingroup\$ And it says that opening+closing another connection clears the connection pool? I wouldn't worry about it; basically all it does is prevent a lag when you're re-connecting with the same connection string; ADODB and its connection pool certainly dies along with your program anyway. \$\endgroup\$ Jan 11 '16 at 12:39
  • \$\begingroup\$ @Mat'sMug ok - for the sake of simplicity and future readability I will not worry about the connection pool - maybe if the class ever needs to expand to a very complex scenario that hits the db with lots of different sql strings then I might need to reconsider. \$\endgroup\$
    – whytheq
    Jan 11 '16 at 13:20

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.