VBA-Sync code feedback

Question

I was hoping to get some input on a class module I'm designing to abstract away the boilerplate of utilizing asynchronous queries in VBA. cQueryable supports both synchronous and asynchronous queries. So you could do something like call a package to populate temp tables. This would be done synchronously because you'd want this to be completed before you executed your select queries. After, you would then execute select queries on each of the temp tables asynchronously.

This code really just abstracts away a lot of the functionality in the ADODB library. I tried to name my properties and methods similarly to what the objects in that library use where possible. My connectionString property is named similarly to the same one in the ADODB.Connection object. And my CreateParam method is named similarly to the createParameter method of the ADODB.Command object.

A few of the new procedures I've introduced are the sql property. This holds the sql query to be executed (this maps to commandtext in the command object). Another is ProcedureAfterQuery. This is to hold the name procedure to be called by the connection object after it raises an event when the query completes. Others are SyncExecute and AsyncExecute which should describe what they do in their names.

One thing to note about these two is that SyncExecute is a function whereas AsyncExecute is a subroutine. I wanted SyncExecute to return a recordset when it completes. But for AsyncExecute, I wanted it to be a sub I didn't want to imply that it returned anything. I use similar (but different) code to do this. So I think I violate the DRY principle. I could consolidate these two to call one shared subroutine procedure. That shared procedure would then be more complicated, but the code would at least be shared. I don't have a preference one way or another.

Although CreateParam is similar to the CreateParameter method of the command object, there's two differences. One is that the order of the arguments is different. This is mainly because the size and direction parameters are listed as optional parameters with default values. Their default values can just be used when the value is numeric, but size must be specified if the value is a string. So in certain situations, size is optional, whereas in others it's required. And the query will fail if it isn't provided.

Other things I didn't consider (or test) is that I've read that ADODB can be used essentially anywhere a driver can be provided. So this could be used on Excel workbooks, perhaps text files, and other sources rather than just databases. So perhaps the synchronous and asynchronous queries would work there as well. But that's not what I set out to design or test.

I'd appreciate constructive criticism.

VERSION 1.0 CLASS
BEGIN
 MultiUse = -1 'True
END
Attribute VB_Name = "cQueryable"
Attribute VB_GlobalNameSpace = False
Attribute VB_Creatable = False
Attribute VB_PredeclaredId = False
Attribute VB_Exposed = False
Option Explicit
'Requires a refernce to the Microsoft ActiveX Data Objects 6.1 Library (or equivalent)
Private WithEvents mASyncConn As ADODB.Connection
Attribute mASyncConn.VB_VarHelpID = -1
Private mSyncConn As ADODB.Connection
Private mConn As ADODB.Connection
Private mComm As ADODB.Command
Private mSql As String
Private mProcedureAfterQuery As String
Private mAsync As Boolean
Private mConnectionString As String
Private Const mSyncExecute As Long = -1
Private Sub Class_Initialize()
 Set mComm = New ADODB.Command
 Set mConn = New ADODB.Connection
End Sub
Public Property Let Sql(value As String)
 mSql = value
End Property
Public Property Get Sql() As String
 Sql = mSql
End Property
Public Property Let ConnectionString(value As String)
 mConnectionString = value
End Property
Public Property Get ConnectionString() As String
 ConnectionString = mConnectionString
End Property
Public Property Let procedureAfterQuery(value As String)
 mProcedureAfterQuery = value
End Property
Public Property Get procedureAfterQuery() As String
 procedureAfterQuery = mProcedureAfterQuery
End Property
Public Sub createParam(pName As String, pType As DataTypeEnum, pValue As Variant, Optional pDirection As ParameterDirectionEnum = adParamInput, Optional pSize As Long = 0)
 Dim pm As ADODB.Parameter
 With mComm
 Set pm = .CreateParameter(name:=pName, Type:=pType, direction:=pDirection, value:=pValue, size:=pSize)
 .Parameters.Append pm
 End With
End Sub
Public Function SyncExecute()
 Set mSyncConn = mConn
 If connectionSuccessful Then
 With mComm
 .CommandText = mSql
 Set .ActiveConnection = mSyncConn
 Set SyncExecute = .execute(Options:=mSyncExecute)
 End With
 End If
End Function
Public Sub AsyncExecute()
 Set mASyncConn = mConn
 If connectionSuccessful Then
 With mComm
 .CommandText = mSql
 Set .ActiveConnection = mASyncConn
 .execute Options:=adAsyncExecute
 End With
 End If
End Sub
Private Function connectionSuccessful() As Boolean
 If mConn.State = adStateClosed Then
 mConn.ConnectionString = mConnectionString
 End If
 
 On Error GoTo errHandler
 If mConn.State = adStateClosed Then
 mConn.Open
 End If
 
 connectionSuccessful = (mConn.State = adStateOpen)
 On Error GoTo 0
 
 Exit Function
errHandler:
 Debug.Print "Error: Connection unsuccessful"
 connectionSuccessful = False
End Function
Private Sub mASyncConn_ExecuteComplete(ByVal RecordsAffected As Long, ByVal pError As ADODB.Error, adStatus As ADODB.EventStatusEnum, ByVal pCommand As ADODB.Command, ByVal pRecordset As ADODB.Recordset, ByVal pConnection As ADODB.Connection)
 If mProcedureAfterQuery <> "" Then
 Call Application.Run(mProcedureAfterQuery, pRecordset)
 End If
End Sub

• TinMan: In order to respond to events of an object in a scripting text file you will need to declare the library and type of object. The only way this can be done is using CreateObject. For example Set Connection = WScript.CreateObject("ADODB.Connection", "Connection_"). With this setup Sub Connection_ExecuteComplete(RecordsAffected, pError, adStatus, pCommand, pRecordset, pConnection) will fire correctly. But I'm not sure that this will work in a scripting class. Hopefully, with a little duck, one of the real Gurus can elaborate on this.
• beyphy: Ah I have no idea. Thanks for the heads up though. My code wasn't really meant to handle a scripting text file. So if it doesn't work I'll just put it as a caveat on my github.
• ARickman: @beyphy I have had to use the ADODB library so extensively that I have created several of my own wrapper classes. Overtime I have improved them, from an architectural standpoint and by documenting and addressing the various bugs in the library. It took many man hours to get to the point I have it at now, so I figure it is worth sharing with others so they don't have to do the same. Saying that, here is the github link to the code: github.com/rickmanalexander/ADODBDataAccessAPI.

Answer 1

Private Function connectionSuccessful() As Boolean

The name suggest that you are testing to see if the Connection has already been opened when in fact it is used to opening the Connection and test if it was successful.

Private Function OpenConnection() As Boolean 

This name tells you that you are opening a Connection. Since the return type is Boolean, it is natural to assume that the function will return True only if the Connection was successful.

Having the error handler escape errors and print a message to the Immediate Window is counter productive. As a developer I don't instinctively look to the Immediate Window for error messages. As a user I will notify the developer of the error message that was raised down the line and not at the point of impact. Considering that your code uses callback procedures, there is no guarantee that an error will ever be raised. The only thing that is certain is that there are going to be problems somewhere down the line.

You should definitely raise a custom error it the mConnectionString is not set. A custom error message for the failed connection is not necessary (if you remove the error handler) because an ADODB error will be thrown at the point where this procedure was called.

Public Sub AsyncExecute()

Consider raising an error if the callback procedure is not set.

Private Sub Class_Terminate()

This method should be used to close the connection.

mConn, mASyncConn, and mSyncConn

There is no need to use three different Connection variable. You are doing more work and obfuscating the code. Using a variable such as AsyncMode As Boolean will give you the same feedback and simplify the code making it easier to read.

Naming Conventions

Having value and execute lower case changes the case for all other variables and properties with the same names. For this reason, I use Pascal Case for all my variables that do not have some sort of prefix.

Mathieu Guindon's Factories: Parameterized Object Initialization

Other possible improvements

A public event would allow you to use cQueryable in other custom classes.

Public Event AsyncExecuteComplete(pRecordset As Recordset)

The ability to chain queries together seems like a natural fit.

Public Function NextQuery(Queryable AS cQueryable) AS cQueryable
 Set NextQuery = Queryable 
 Set mQueryable = Queryable 
End Function

This will allow you to run multiple queries in order without the need of multiple callback.

CreateTempQuery.NextQuery(FillTempTableQuery).NextQuery(UpdateEmployeesTableQuery)

• beyphy: Thanks! For the note on the connection variables, I need at least two (the async and the non-async one) to prevent the event from firing each time an execution happens. So I thought to use three to keep things consistent. I thought about potentially creating a method with a paramArray() that accepted multiple SQL queries. I believe the connection is closed when the object gets dereferenced. One issue I think happens is that, if there's an exception, the connection stays open. This makes sense because the object is still referenced. So perhaps I should add error handling to address that case.
• TinMan: @beyphy I would still prefer to let the event fire and do nothing if AsyncMode = False. You should rename procedureAfterQuery to procedureAfterAsyncQuery if you don't want it to run after execution has completed.