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
\$\endgroup\$
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.

\$\endgroup\$
  • 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\$ – RubberDuck May 28 '14 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\$ – Mathieu Guindon May 28 '14 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

\$\endgroup\$
  • 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\$ – Mathieu Guindon May 28 '14 at 14:12
  • 1
    \$\begingroup\$ And +1 for IsTrustedConnection \$\endgroup\$ – Mathieu Guindon May 28 '14 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\$ – Malachi May 28 '14 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\$ – Mathieu Guindon May 28 '14 at 14:43
  • 1
    \$\begingroup\$ Hahahaha. I'll take that as a "yes, that's sufficient to break conventional wisdom", @Mat'sMug. \$\endgroup\$ – RubberDuck May 28 '14 at 14:51

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.