9
\$\begingroup\$

I recently wrote a DSN class for use with the Access flavor of VBA. I'm preparing to refactor and would appreciate feedback.

I am aware of two issues.

  1. I added the findSectionKey() function to the Registry class (which is separate and out of scope for this question) that I call, but never went back to remove the duplicate logic from my DSN class. I am aware of this and will correct it during my refactor.
  2. I am completely unsatisfied with the way I implemented the Driver property. I use an Enum to specify the DriverType and then a select statement to return a string from the aforementioned Driver property. It's works well in practice, but is kind of clunky to work with (particularly when adding support for a new driver). I'm thinking I could use a Dictionary to do this better, but I've never used one and I'm unsure if that's the best approach.

So, I am concerned with Issue #2 first and foremost, but all feedback is welcome.

' ***************************************************************************
' * Class: DSN *
' * Author: Christopher J. McClellan *
' * Published under Creative Commons Attribution-Share Alike *
' * http://creativecommons.org/licenses/by-sa/3.0/ *
' * *
' * Allows for easy creation of DSN entries in the ODBC Administrator by *
' * leveraging the Registry class. *
' * Entries in the ODBC Admin are not created until the *
' * Create() sub is called. *
' ***************************************************************************
Private Const CLASS_NAME As String = "DSN" 'for error handling
Public Enum eDSN_Bitmode
 DSN_64BIT = 0
 DSN_32BIT = 1
End Enum
Private Const REG_KEYNODE_32 As String = "software\wow6432Node\ODBC"
Private Const REG_KEYNODE_64 As String = "software\ODBC"
Public Enum eDSN_type
 DSN_SYSTEM = 0
 DSN_USER = 1
End Enum
Public Enum eDSN_Driver
 DSN_DRIVER_EMPTY = 0
 DSN_DRIVER_SQLSERVER = 1
 DSN_DRIVER_SQLSERVER10 = 2
 DSN_DRIVER_SQLSERVER11 = 3
 DSN_DRIVER_ORA11G = 11
End Enum
Private Const DSN_DRIVER_SQLSERVER_NAME As String = "SQL Server"
Private Const DSN_DRIVER_SQLSERVER10_NAME As String = "SQL Server Native Client 10.0"
Private Const DSN_DRIVER_SQLSERVER11_NAME As String = "SQL Server Native Client 11.0"
Private Const DSN_DRIVER_ORA11G_NAME As String = "Oracle in OraClient11g_home1"
Public Enum eORA_CommitMode
 ORA_COMMIT_IfSuccessful
 ORA_COMMIT_FirstFailure
 ORA_COMMIT_AllSuccessful
End Enum
Private Const ORA_COMMIT_IfSuccessful_Txt As String = "IfAllSuccessful"
Private Const ORA_COMMIT_FirstFailure_Txt As String = "UpToFirstFailure"
Private Const ORA_COMMIT_AllSuccessful_Txt As String = "AllSuccessful"
Public Enum eORA_NumericSetting
 ORA_NUMSET_OracleNLS
 ORA_NUMSET_MSRegional
 ORA_NUMSET_US
End Enum
Private Const ORA_NUMSET_OracleNLS_txt As String = "NLS"
Private Const ORA_NUMSET_MSRegional_txt As String = "MS"
Private Const ORA_NUMSET_US_txt As String = "US"
' shared props
Private mName As String
Private mDriver As eDSN_Driver
Private mDSNType As eDSN_type
Private mBitMode As eDSN_Bitmode
Private mDriverFile As String
Private mDesc As String
' sql server props
Private mServer As String
Private mDatabase As String
Private mTrustedConnection As Boolean
' ora props
Private mAppAttributes As Boolean
Private mReadOnlyConn As Boolean
Private mCommitMode As eORA_CommitMode
Private mBindAsDate As Boolean
Private mBindAsFloat As Boolean
Private mCacheBufferSize As Long
Private mCloseCursor As Boolean
Private mDisableDPM As Boolean
Private mDisableMTS As Boolean
Private mDisableRuleHint As Boolean
Private mExecSchemaOpt As String
Private mExecSyntax As Boolean
Private mFailover As Boolean
Private mFailoverDelay As Long
Private mFailoverRetryCount As Integer
Private mFetchBufferSize As Long
Private mForceWChar As Boolean
Private mLobs As Boolean
Private mMetaDataIdDefault As Boolean
Private mNumericSetting As eORA_NumericSetting
Private mQueryTimeout As Boolean
Private mResultSets As Boolean
Private mStatementCache As Boolean
Private mUserID As String
Private mPwd As String
' defaults
Private Const DSN_DFLT_CacheBufferSize As Long = 20
Private Const DSN_DFLT_FailoverDelay As Long = 10
Private Const DSN_DFLT_FailoverRetryCount As Integer = 10
Private Const DSN_DFLT_FetchBufferSize As Long = 64000
''''''''''''''''''''''''''''BEGIN SHARED PROPERTIES''''''''''''''''''''''''''''''
Property Get NAME() As String
 NAME = mName
End Property
Property Let NAME(str As String)
 mName = str
End Property
Property Get Driver() As eDSN_Driver
' defaults to empty via enum
 Driver = mDriver
End Property
Property Let Driver(eDriver As eDSN_Driver)
 mDriver = eDriver
End Property
Property Get DriverName() As String
' read only property
Select Case mDriver
 Case DSN_DRIVER_EMPTY
 DriverName = ""
 Case DSN_DRIVER_SQLSERVER
 DriverName = DSN_DRIVER_SQLSERVER_NAME
 Case DSN_DRIVER_SQLSERVER10
 DriverName = DSN_DRIVER_SQLSERVER10_NAME
 Case DSN_DRIVER_SQLSERVER11
 DriverName = DSN_DRIVER_SQLSERVER11_NAME
 Case DSN_DRIVER_ORA11G
 DriverName = DSN_DRIVER_ORA11G_NAME
 End Select
End Property
Property Get DSNType() As eDSN_type
 DSNType = mDSNType
End Property
Property Let DSNType(pDSNType As eDSN_type)
 mDSNType = pDSNType
End Property
Property Get BitMode() As eDSN_Bitmode
 BitMode = mBitMode
End Property
Property Let BitMode(pBitMode As eDSN_Bitmode)
 ' defaults to 64 bit via the enum
 mBitMode = pBitMode
End Property
Property Get DriverFile() As String
' read only property
' @ todo - optimize
' save effort by only checking the registry the first time
 'If mDriverFile = "" Then
 mDriverFile = getDriver
 DriverFile = mDriverFile
 'Else
 ' DriverFile = mDriverFile
 'End If
End Property
Public Property Get Description() As String
 Description = mDesc
End Property
Public Property Let Description(desc As String)
 mDesc = desc
End Property
Property Get Server() As String
 Select Case Me.Driver
 Case DSN_DRIVER_EMPTY
 'Server = Err_DriverNotSet
 ErrRaise_DriverNotSet
 Case DSN_DRIVER_ORA11G
 Server = Me.NAME
 Case Else
 Server = mServer
 End Select
End Property
Property Let Server(str As String)
 If Me.Driver <> DSN_DRIVER_ORA11G Then
 mServer = str
 Else
 ErrRaise_NotSupported
 End If
End Property
Private Property Get RegNode() As String
' read only property
 Select Case Me.BitMode
 Case DSN_32BIT
 RegNode = REG_KEYNODE_32
 Case DSN_64BIT
 RegNode = REG_KEYNODE_64
 Case Else
 Err.Raise vbObjectError + 25002, CurrentProject.NAME & "." & CLASS_NAME, "Invalid Bitmode"
 End Select
End Property
Private Property Get HKey() As Long
' read only property
' determine which HKEY to use
 Select Case Me.DSNType
 Case DSN_USER
 HKey = HKEY_CURRENT_USER
 Case DSN_SYSTEM
 HKey = HKEY_LOCAL_MACHINE
 Case Else
 Err.Raise vbObjectError + 25001, CurrentProject.NAME & "." & CLASS_NAME, "Invalid DSNType or DSNType not set"
 End Select
End Property
''''''''''''''''''''''''''''''END SHARED PROPERTIES''''''''''''''''''''''''
''''''''''''''''''''''''''''BEGIN ORA PROPERTIES'''''''''''''''''''''''''''
Public Property Get AppAttributes() As Boolean
 Select Case Me.Driver
 Case DSN_DRIVER_EMPTY
 AppAttributes = False
 ErrRaise_DriverNotSet
 Case DSN_DRIVER_ORA11G
 AppAttributes = mAppAttributes
 Case Else
 AppAttributes = False
 ErrRaise_NotSupported
 End Select
End Property
Public Property Let AppAttributes(bool As Boolean)
 If Me.Driver = DSN_DRIVER_ORA11G Then
 mAppAttributes = bool
 Else
 mAppAttributes = False
 ErrRaise_NotSupported
 End If
End Property
Public Property Get ReadOnlyConnection() As Boolean
'''''''''''''''''''''''''''''''''''''''''''''''''''''
' Attributes '
' true = "R" (read); false = "W" (write) '
'''''''''''''''''''''''''''''''''''''''''''''''''''''
 Select Case Me.Driver
 Case DSN_DRIVER_EMPTY
 ReadOnlyConnection = True 'return true as a precaution
 ErrRaise_DriverNotSet
 Case DSN_DRIVER_ORA11G
 ReadOnlyConnection = mReadOnlyConn
 Case Else
 ReadOnlyConnection = True
 ErrRaise_NotSupported
 End Select
End Property
Public Property Let ReadOnlyConnection(bool As Boolean)
 Select Case Me.Driver
 Case DSN_DRIVER_EMPTY
 ErrRaise_DriverNotSet
 Case DSN_DRIVER_ORA11G
 mReadOnlyConn = bool
 Case Else
 ErrRaise_NotSupported
 End Select
End Property
Public Property Get BatchAutoCommitMode() As eORA_CommitMode
 Select Case Me.Driver
 Case DSN_DRIVER_EMPTY
 ErrRaise_DriverNotSet
 Case DSN_DRIVER_ORA11G
 BatchAutoCommitMode = mCommitMode
 Case Else
 ErrRaise_NotSupported
 End Select
End Property
Public Property Let BatchAutoCommitMode(mode As eORA_CommitMode)
 Select Case Me.Driver
 Case DSN_DRIVER_EMPTY
 ErrRaise_DriverNotSet
 Case DSN_DRIVER_ORA11G
 mCommitMode = mode
 Case Else
 ErrRaise_NotSupported
 End Select
End Property
Public Property Get BindAsDate() As Boolean
 Select Case Me.Driver
 Case DSN_DRIVER_EMPTY
 ErrRaise_DriverNotSet
 Case DSN_DRIVER_ORA11G
 BindAsDate = mBindAsDate
 Case Else
 ErrRaise_NotSupported
 End Select
End Property
'.... lots of properties with the same logic omitted due to space constraints
' @todo - add sql server support for sql server login
Public Property Get USERID() As String
 Select Case Me.Driver
 Case DSN_DRIVER_EMPTY
 ErrRaise_DriverNotSet
 Case DSN_DRIVER_ORA11G
 USERID = mUserID
 Case Else
 ErrRaise_NotSupported
 End Select
End Property
Public Property Let USERID(uid As String)
 Select Case Me.Driver
 Case DSN_DRIVER_EMPTY
 ErrRaise_DriverNotSet
 Case DSN_DRIVER_ORA11G
 mUserID = uid
 Case Else
 ErrRaise_NotSupported
 End Select
End Property
' @todo - add sql server support for sql server login
Public Property Get Password() As String
 Select Case Me.Driver
 Case DSN_DRIVER_EMPTY
 ErrRaise_DriverNotSet
 Case DSN_DRIVER_ORA11G
 Password = mPwd
 Case Else
 ErrRaise_NotSupported
 End Select
End Property
Public Property Let Password(pwd As String)
 Select Case Me.Driver
 Case DSN_DRIVER_EMPTY
 ErrRaise_DriverNotSet
 Case DSN_DRIVER_ORA11G
 mPwd = pwd
 Case Else
 ErrRaise_NotSupported
 End Select
End Property
''''''''''''''''''''''''''''''END ORA PROPERTIES'''''''''''''''''''''''''''
''''''''''''''''''''''''BEGIN SQL SERVER PROPERTIES''''''''''''''''''''''''
Property Get Database() As String
 Select Case Me.Driver
 Case DSN_DRIVER_EMPTY
 'Database = Err_DriverNotSet
 ErrRaise_DriverNotSet
 Case DSN_DRIVER_ORA11G
 'Database = Err_NotSupported
 ErrRaise_NotSupported
 Case Else
 Database = mDatabase
 End Select
End Property
Property Let Database(pDatabase As String)
 If Me.Driver <> DSN_DRIVER_ORA11G Then
 mDatabase = pDatabase
 Else
 'mDatabase = Err_NotSupported
 ErrRaise_NotSupported
 End If
End Property
Property Get TrustedConnection() As Boolean
 TrustedConnection = mTrustedConnection
End Property
Property Let TrustedConnection(value As Boolean)
 mTrustedConnection = value
End Property
'''''''''''''''''''''''END SQL SERVER PROPERTIES'''''''''''''''''''''''''''''''''''''
Private Function getDriver() As String
'**********************************************
' returns the file path of the .dll driver file
'**********************************************
Const PROC_NAME As String = "getDriver"
On Error GoTo ErrHandler:
 If Me.Driver = DSN_DRIVER_EMPTY Then
 getDriver = ""
 Else
 'determine bitmode
 Dim skey As String
 skey = RegNode
 ' find ODBC node
 Dim odbcDriverNode As String
 odbcDriverNode = findSectionKey(HKEY_LOCAL_MACHINE, skey, Me.DriverName)
 If odbcDriverNode <> "" Then
 'create new registry object and find the driver
 Dim reg As New Registry
 With reg
 .ClassKey = HKEY_LOCAL_MACHINE
 .sectionKey = odbcDriverNode
 .ValueKey = "Driver"
 If .value = "" Then
 Err.Raise vbObjectError + 25003, CurrentProject.NAME & "." & CLASS_NAME, "Driver Not Found"
 Else
 'return
 getDriver = .value
 End If
 End With
 Else
 Err.Raise vbObjectError + 25003, CurrentProject.NAME & "." & CLASS_NAME, "Driver Not Found"
 End If
 End If
ExitFunction:
 If Not (reg Is Nothing) Then
 Set reg = Nothing
 End If
 Exit Function
ErrHandler:
 errBox CLASS_NAME, PROC_NAME
 Resume ExitFunction:
End Function
Private Function findSectionKey(regClassKey As ERegistryClassConstants, sectToLookIn As String, sectToFind As String) As String
'******************************************************************
' returns full section key as string
' if a matching section key is not found, returns an empty string
'******************************************************************
On Error GoTo ErrHandler:
Const PROC_NAME As String = "findSectionKey"
 Dim sSect() As String ' string array of subnodes
 Dim iSectCount As Long ' length of sSect array
 Dim reg As New Registry
 With reg
 .ClassKey = regClassKey
 .sectionKey = sectToLookIn
 .EnumerateSections sSect, iSectCount
 Dim i As Long
 For i = 1 To iSectCount
 'Debug.Print .sectionKey & "\" & sSect(i)
 If findSectionKey = "" Then
 If sSect(i) = sectToFind Then
 ' found node
 findSectionKey = .sectionKey & "\" & sSect(i)
 Exit For
 Else
 'search subnodes via recursion
 findSectionKey = findSectionKey(regClassKey, .sectionKey & "\" & sSect(i), sectToFind)
 End If
 Else
 Exit For
 End If
 Next i
 End With
ExitFunction:
 If Not (reg Is Nothing) Then
 Set reg = Nothing
 End If
 Exit Function
ErrHandler:
 errBox CLASS_NAME, PROC_NAME
End Function
Public Sub Create(Optional overwrite As Boolean = True)
' ***************************************************************************************************
' Creates a new registry entry defining the DSN (or modifies existing one with matching reg key)
' The actual registry entry (thus the DSN in the odbc admin) is not created until this sub is called.
' ***************************************************************************************************
 Select Case Me.Driver
 Case DSN_DRIVER_EMPTY
 ErrRaise_DriverNotSet
 Case DSN_DRIVER_ORA11G
 createORA overwrite
 Case Else 'some sql server version
 createSQL overwrite
 End Select
End Sub
Private Sub createSQL(ByVal overwrite As Boolean)
 Dim reg As New Registry
 Dim SCTKEY As String
 Dim isRegKey As Boolean
 ' Registry Section Key
 SCTKEY = RegNode & "\odbc.ini\" & Me.NAME
 ' Create New Key and set values
 With reg
 .ClassKey = HKey
 .sectionKey = SCTKEY
 ' Check existance and overwrite value
 isRegKey = .KeyExists
 If overwrite = True Or (overwrite = False And isRegKey = False) Then
 'driver
 .ValueKey = "Driver"
 .ValueType = REG_SZ
 .value = Me.DriverFile
 'server
 .ValueKey = "Server"
 .ValueType = REG_SZ
 .value = Me.Server
 'database
 .ValueKey = "Database"
 .ValueType = REG_SZ
 .value = Me.Database
 'trusted connection
 .ValueKey = "Trusted_Connection"
 .ValueType = REG_SZ
 If Me.TrustedConnection = True Then
 .value = "Yes"
 Else
 .value = "No"
 End If
 'last user
 .ValueKey = "LastUser"
 .ValueType = REG_SZ
 .Value = Environ("UserName")
 ' Description
 .ValueKey = "Description"
 .ValueType = REG_SZ
 .value = Me.Description
 End If
 End With
 ' add a registry key to make the newly added DSN visible
 If overwrite = True Or (overwrite = False And isRegKey = False) Then
 addDataSourceEntry
 End If
End Sub
Private Sub createORA(ByVal overwrite As Boolean)
 Dim reg As New Registry
 Dim SCTKEY As String
 Dim isRegKey As Boolean
 ' Registry Section Key
 SCTKEY = RegNode & "\odbc.ini\" & Me.NAME
 ' Create New Key and set values
 With reg
 .ClassKey = HKey
 .sectionKey = SCTKEY
 ' Check existance and overwrite value
 isRegKey = .KeyExists
 If overwrite = True Or (overwrite = False And isRegKey = False) Then
 .ValueKey = "Application Attributes"
 .ValueType = REG_SZ
 .value = boolAsString(Me.AppAttributes)
 .ValueKey = "Attributes"
 .ValueType = REG_SZ
 If Me.ReadOnlyConnection Then
 .value = "R"
 Else
 .value = "W"
 End If
 .ValueKey = "BatchAutocommitMode"
 .ValueType = REG_SZ
 Select Case Me.BatchAutoCommitMode
 Case ORA_COMMIT_AllSuccessful
 .value = ORA_COMMIT_AllSuccessful_Txt
 Case ORA_COMMIT_FirstFailure
 .value = ORA_COMMIT_FirstFailure_Txt
 Case ORA_COMMIT_IfSuccessful
 .value = ORA_COMMIT_IfSuccessful_Txt
 End Select
 .ValueKey = "BindAsDATE"
 .ValueType = REG_SZ
 .value = boolAsString(Me.BindAsDate)
 .ValueKey = "BindAsFLOAT"
 .ValueType = REG_SZ
 .value = boolAsString(Me.BindAsFloat)
 .ValueKey = "CacheBufferSize"
 .ValueType = REG_SZ
 .value = Me.CacheBufferSize
 .ValueKey = "CloseCursor"
 .ValueType = REG_SZ
 .value = boolAsString(Me.CloseCursor)
 .ValueKey = "Description"
 .ValueType = REG_SZ
 .value = Me.Description
 .ValueKey = "DisableDPM"
 .ValueType = REG_SZ
 .value = boolAsString(Me.DisableDPM)
 .ValueKey = "DisableMTS"
 .ValueType = REG_SZ
 .value = boolAsString(Me.DisableMTS)
 .ValueKey = "DisableRULEHint"
 .ValueType = REG_SZ
 .value = boolAsString(Me.DisableRULEHint)
 .ValueKey = "Driver"
 .ValueType = REG_SZ
 .value = Me.DriverFile
 .ValueKey = "DSN"
 .ValueType = REG_SZ
 .value = Me.NAME
 .ValueKey = "EXECSchemaOpt"
 .ValueType = REG_SZ
 .value = Me.EXECSchemaOpt
 .ValueKey = "EXECSyntax"
 .ValueType = REG_SZ
 .value = boolAsString(Me.EXECSyntax)
 .ValueKey = "Failover"
 .ValueType = REG_SZ
 .value = boolAsString(Me.Failover)
 .ValueKey = "FailoverDelay"
 .ValueType = REG_SZ
 .value = Me.FailoverDelay
 .ValueKey = "FailoverRetryCount"
 .ValueType = REG_SZ
 .value = Me.FailoverRetryCount
 .ValueKey = "FetchBufferSize"
 .ValueType = REG_SZ
 .value = Me.FetchBufferSize
 .ValueKey = "ForceWCHAR"
 .ValueType = REG_SZ
 .value = boolAsString(Me.ForceWChar)
 .ValueKey = "Lobs"
 .ValueType = REG_SZ
 .value = boolAsString(Me.Lobs)
 .ValueKey = "MetadataIdDefault"
 .ValueType = REG_SZ
 .value = boolAsString(Me.MetadataIdDefault)
 .ValueKey = "NumericSetting"
 .ValueType = REG_SZ
 Select Case Me.NumericSetting
 Case ORA_NUMSET_OracleNLS
 .value = ORA_NUMSET_OracleNLS_txt
 Case ORA_NUMSET_MSRegional
 .value = ORA_NUMSET_MSRegional_txt
 Case ORA_NUMSET_US
 .value = ORA_NUMSET_US_txt
 End Select
 .ValueKey = "Password"
 .ValueType = REG_SZ
 .value = Me.Password
 .ValueKey = "QueryTimeout"
 .ValueType = REG_SZ
 .value = boolAsString(Me.QueryTimeout)
 .ValueKey = "ResultSets"
 .ValueType = REG_SZ
 .value = boolAsString(Me.ResultSets)
 .ValueKey = "ServerName"
 .ValueType = REG_SZ
 .value = Me.Server
 .ValueKey = "StatementCache"
 .ValueType = REG_SZ
 .value = boolAsString(Me.StatementCache)
 .ValueKey = "UserID"
 .ValueType = REG_SZ
 .value = Me.USERID
 End If
 End With
 ' add a registry key to make the newly added DSN visible
 If overwrite = True Or (overwrite = False And isRegKey = False) Then
 addDataSourceEntry
 End If
End Sub
Private Sub addDataSourceEntry()
 Dim reg As Registry
 Set reg = New Registry
 With reg
 .ClassKey = HKey
 .sectionKey = RegNode & "\odbc.ini\odbc data sources"
 .ValueKey = Me.NAME
 .ValueType = REG_SZ
 .value = Me.DriverName
 End With
End Sub
Public Function Exists() As Boolean
' Assumes Name, DSNType, and Bitmode are set
 Dim reg As New Registry
 Dim SCTKEY As String
 SCTKEY = RegNode & "\odbc.ini\" & Me.NAME
 With reg
 .ClassKey = HKey
 .sectionKey = SCTKEY
 If .KeyExists Then
 Exists = True
 Else
 Exists = False
 End If
 End With
End Function
Public Sub Delete()
 Dim reg As New Registry
 With reg
 .ClassKey = HKey
 'delete registry Section key
 .sectionKey = RegNode & "\odbc.ini\" & Me.NAME
 If Exists Then
 .DeleteKey
 'delete related ODBC Datasource ValueKey
 .sectionKey = RegNode & "\odbc.ini\odbc data sources"
 .ValueKey = Me.NAME
 .DeleteValue
 End If
 End With
End Sub
Private Function boolAsString(ByVal bool As Boolean) As String
 If bool = True Then
 boolAsString = "T"
 Else
 boolAsString = "F"
 End If
End Function
Private Sub Class_Initialize()
' bypass all setters & getters when initializing
 ' shared props
 'mName =
 mDriver = DSN_DRIVER_EMPTY
 mDSNType = DSN_SYSTEM
 mBitMode = DSN_32BIT
 'mDriverFile =
 'mDesc =
 'mServer =
 ' sql server
 'mDatabase =
 mTrustedConnection = True
 ' oracle
 mAppAttributes = False
 mReadOnlyConn = True
 mCommitMode = ORA_COMMIT_IfSuccessful
 mBindAsDate = False
 mBindAsFloat = False
 mCacheBufferSize = DSN_DFLT_CacheBufferSize
 mCloseCursor = False
 mDisableDPM = False
 mDisableMTS = True
 mDisableRuleHint = True
 mExecSchemaOpt = ""
 mExecSyntax = False
 mFailover = True
 mFailoverDelay = DSN_DFLT_FailoverDelay
 mFailoverRetryCount = DSN_DFLT_FailoverRetryCount
 mFetchBufferSize = DSN_DFLT_FetchBufferSize
 mForceWChar = False
 mLobs = True
 mMetaDataIdDefault = False
 mNumericSetting = ORA_NUMSET_OracleNLS
 mQueryTimeout = False
 mResultSets = False
 mStatementCache = False
 ' @todo - make sql serv support sql server login; move to shared
 'mUserID =
 'mPwd =
End Sub
Private Sub errBox(ModuleName As String, procName As String, Optional style As VbMsgBoxStyle = vbCritical)
 MsgBox "Module: " & ModuleName & vbCrLf & _
 "Procedure: " & procName & vbCrLf & _
 Err.Description, _
 style, _
 "Runtime Error: " & Err.number
End Sub
Private Sub ErrRaise_NotSupported()
 Err.Raise vbObjectError + 25010, CurrentProject.NAME & "." & CLASS_NAME, "Driver does not support the property."
End Sub
Private Sub ErrRaise_DriverNotSet()
 Err.Raise vbObjectError + 25020, CurrentProject.NAME & "." & CLASS_NAME, "Driver property is not set."
End Sub
Mathieu Guindon
75.5k18 gold badges194 silver badges467 bronze badges
asked May 28, 2014 at 0:14
\$\endgroup\$

2 Answers 2

7
\$\begingroup\$

This constant is not needed, and if you decided to rename the class to DomainNameService, it would be telling a lie:

Private Const CLASS_NAME As String = "DSN" 'for error handling

Instead, use TypeName(Me) to get the class' name as it appears at runtime.

The class has roughly 800 lines of code. Let's look at its public interface... side note, the inconsistently specified accessibility modifiers made me wonder what the default was in (if it's not specified, it's Public).

So:

Public Property Get Name() As String
Public Property Let Name(String) As String
Public Property Get Driver() As eDSN_Driver
Public Property Let Driver(eDSN_Driver)
Public Property Get DriverName() As String
Public Property Get DSNType() As eSDN_type
Public Property Let DSNType(eDSN_type)
Public Property Get BitMode() As eDSN_Bitmode
Public Property Let BitMode(eDSN_Bitmode)
Public Property Get DriverFile() As String
Public Property Get Description() As String
Public Property Let Description(String)
Public Property Get Server() As String
Public Property Let Server(String)

These "shared properties" are the properties of one type. The "Oracle properties" are members of another type, and the "SQL properties" are members of another type. I mean, I'd put them in 3 classes.


Using an Enum for Driver is an excellent idea, it makes you avoid using magic strings or worse, magic numbers. It makes sense to use a Select Case to switch on an enum:

Property Get DriverName() As String
' read only property
Select Case mDriver
 Case DSN_DRIVER_EMPTY
 DriverName = ""
 Case DSN_DRIVER_SQLSERVER
 DriverName = DSN_DRIVER_SQLSERVER_NAME
 Case DSN_DRIVER_SQLSERVER10
 DriverName = DSN_DRIVER_SQLSERVER10_NAME
 Case DSN_DRIVER_SQLSERVER11
 DriverName = DSN_DRIVER_SQLSERVER11_NAME
 Case DSN_DRIVER_ORA11G
 DriverName = DSN_DRIVER_ORA11G_NAME
 End Select
End Property

However I'd do it like this:

Property Get DriverName() As String
 Select Case mDriver 
 Case DSN_DRIVER_SQLSERVER
 DriverName = DSN_DRIVER_SQLSERVER_NAME
 Case DSN_DRIVER_SQLSERVER10
 DriverName = DSN_DRIVER_SQLSERVER10_NAME
 Case DSN_DRIVER_SQLSERVER11
 DriverName = DSN_DRIVER_SQLSERVER11_NAME
 Case DSN_DRIVER_ORA11G
 DriverName = DSN_DRIVER_ORA11G_NAME
 Case Else
 DriverName = vbNullString
 End Select
End Property

The difference? Vertical breathing whitespace, an explicit default value for anything that doesn't fit a Case block, and vbNullString being used instead of "". It's just a little technicality, but consider this - "" is not equivalent to vbNullString:

?lenb(vbnullstring), lenb("")
 0 0 
?strptr(vbnullstring), strptr("")
 0 56023156 

If you really don't like maintaining a SELECT CASE, you could be maintaining a Dictionary instead (see this post for the implementation I'm referring to):

Private DriverNames As Dictionary
Private Sub InitializeDriverNames()
 DriverNames = New Dictionary ' this also works with a Scripting.Dictionary
 DriverNames.Add DSN_DRIVER_SQLSERVER, DSN_DRIVER_SQLSERVER_NAME
 DriverNames.Add DSN_DRIVER_SQLSERVER10, DSN_DRIVER_SQLSERVER10_NAME
 DriverNames.Add DSN_DRIVER_SQLSERVER11, DSN_DRIVER_SQLSERVER11_NAME
 DriverNames.Add DSN_DRIVER_ORA11G, DSN_DRIVER_ORA11_NAME
End Sub

And then you can add a call to InitializeDriverNames in Class_Initialize(), and the DriverName getter can look like this:

Public Property Get DriverName() As String
 Dim outResult As String
 If DriverNames.TryGetValue(mDriver, outResult) Then DriverName = outResult
 'If DriverNames.Exists(mDriver) Then DriverName = DriverNames.Item(mDriver)
End Property

The commented-out line shows how to make it work with a Scripting.Dictionary.


I like these two:

Private Sub ErrRaise_NotSupported()
 Err.Raise vbObjectError + 25010, CurrentProject.NAME & "." & CLASS_NAME, "Driver does not support the property."
End Sub
Private Sub ErrRaise_DriverNotSet()
 Err.Raise vbObjectError + 25020, CurrentProject.NAME & "." & CLASS_NAME, "Driver property is not set."
End Sub

However I'd write them like this:

Private Sub RaiseNotSupportedError()
 Err.Raise vbObjectError + 25010, CurrentProject.NAME & "." & TypeName(Me), "Driver does not support the property."
End Sub
Private Sub RaiseDriverNotSetError()
 Err.Raise vbObjectError + 25020, CurrentProject.NAME & "." & TypeName(Me), "Driver property is not set."
End Sub

And errBox should be called ShowErrorBox or similar, i.e. it should start with a verb, too.


Avoid comparing a Boolean with True or False like this:

If bool = True Then

boolAsString could be written like this:

Private Function boolAsString(ByVal value As Boolean) As String
 boolAsString = IIf(value, "T", "F")
End Function

IIf is generally frowned upon because both statements get evaluated no matter what, but in trivial string assignations like this, it doesn't matter.

...OTOH, food for thought:

If Me.TrustedConnection = True Then
 .value = "Yes"
Else
 .value = "No"
End If

There's a lot more to say about your code, but I'd strongly recommend you extract the SQL-specifics into its own class, and do the same for the Oracle-specifics - you'll have 3 shorter, more cohesive classes.

answered May 28, 2014 at 2:49
\$\endgroup\$
2
  • 1
    \$\begingroup\$ I could kiss you for pointing me to TypeName()! Thank you very much for taking the time. I guess it's time to break down and learn how to implement an interface. \$\endgroup\$ Commented May 28, 2014 at 10:00
  • 1
    \$\begingroup\$ Note, I started listing the public members that make up your interface, and stopped when I saw the END SHARED PROPERTIES banner - I meant interface as in what's publicly accessible for that type, not as IInterface... although you could probably use an IDataDriver interface that exposes a Create method, and implement it with some Oracle11DataDriver, SqlServerDataDriver, SqlServer10DataDriver and SqlServer11DataDriver classes.. but I think there's more restructuring that needs to happen before there can be advantages in doing that ;) \$\endgroup\$ Commented May 28, 2014 at 14:57
4
\$\begingroup\$

this little bit of code should be changed

Property Let Server(str As String)
 If Me.Driver <> DSN_DRIVER_ORA11G Then
 mServer = str
 Else
 ErrRaise_NotSupported
 End If
End Property

I would change this around so that you are using a positive conditional, get straight to the point

Property Let Server(str As String)
 If Me.Driver = DSN_DRIVER_ORA11G Then
 ErrRaise_NotSupported
 Else
 mServer = str
 End If
End Property

I don't know if either is faster, but this is the standard way of writing if blocks in any coding language, when it doesn't matter order of the operations write them with positive conditional statements.


Same thing here

Property Let Database(pDatabase As String)
 If Me.Driver <> DSN_DRIVER_ORA11G Then
 mDatabase = pDatabase
 Else
 'mDatabase = Err_NotSupported
 ErrRaise_NotSupported
 End If
End Property

should be

Property Let Database(pDatabase As String)
 If Me.Driver = DSN_DRIVER_ORA11G Then
 'mDatabase = Err_NotSupported
 ErrRaise_NotSupported
 Else
 mDatabase = pDatabase
 End If
End Property

Your naming scheme could use a tune up as well.

Property Get TrustedConnection() As Boolean
 TrustedConnection = mTrustedConnection
End Property
Property Let TrustedConnection(value As Boolean)
 mTrustedConnection = value
End Property

TrustedConnection as a boolean should be isTrustedConnection this way when ever you see it in your code you know it is a boolean, it makes your code easier to read.


More to come

answered May 28, 2014 at 14:00
\$\endgroup\$
6
  • 2
    \$\begingroup\$ I'd normally agree with the positive conditionals, but it turns out there is a reason for the condition to be reversed: you want the If branch to be your "happy path", and the Else branch to be the "not-so-happy path" - which is what the OP has. I guess we can meet halfway and say If Not Me.Driver = DSN_DRIVER_ORA11G Then {happy path} Else {unhappy path} End If ;) \$\endgroup\$ Commented May 28, 2014 at 14:12
  • 1
    \$\begingroup\$ And +1 for IsTrustedConnection \$\endgroup\$ Commented May 28, 2014 at 14:14
  • 1
    \$\begingroup\$ I agree with If Not Me.Driver = DSN_DRIVER_ORA11G Then {happy path} Else {unhappy path} End If @Mat'sMug \$\endgroup\$ Commented May 28, 2014 at 14:27
  • 2
    \$\begingroup\$ @ckuhn203 you're wrapping the ODBC manager; it's your API - you make your own conventions! If you want them consistently matching with the ODBC manager's naming (say, because rest of team is used to that API), then stick to it. Otherwise, you're building an abstraction on top of it, so you may use whatever conventions you want :) \$\endgroup\$ Commented May 28, 2014 at 14:43
  • 1
    \$\begingroup\$ Hahahaha. I'll take that as a "yes, that's sufficient to break conventional wisdom", @Mat'sMug. \$\endgroup\$ Commented May 28, 2014 at 14:51

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.