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 vba (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.