Skip to main content
replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

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

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):

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):

added 10 characters in body
Source Link
Mathieu Guindon
  • 75.1k
  • 18
  • 191
  • 463

The difference? Vertical breathing whitespace, an explicit default value for anything that'sthat 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:

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):

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

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:

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:

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):

Source Link
Mathieu Guindon
  • 75.1k
  • 18
  • 191
  • 463

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's 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.