Class to retrieve external data

I have recently come to use VBA class modules in my code.

Based in part on an answer on Stack Overflow, I bought VBA Developer's Handbook and Professional Excel Development, but it took me a long time to 'get' classes (though I still have a long way to go).

I wrote myself a GetExternalData class, as one of the very common tasks I undertake in Excel VBA is incorporating data from other sources into my reports.

My thought was that the class could take care of deciding the best way of getting the data, based on the source and how the data is to be used, so my getExternalData method was using a 'Variant' so that I could use the same variable whether I return a Collection, Array, Dictionary or Worksheet.

However, when I attempted to pass the Collection to another function which expects a collection I was getting an error. I noticed the locals window showed as 'Variant/Object/Collection', so I tried setting the value to an Object rather than a Variant which works.

My question on Stack Overflow was actually about this practice (Dim as Object... set = Collection).

TODO:

-
At the moment, I don't have anything coded for Access database connections

-
I check the first element of a collection, and assume all items are similar types and have the same number of elements (for arrays)

-
I know there's debate on the merits of Hungarian - at the moment, I find it useful (have been using it for a long time in Access (tbl, qry) and in VBA user-forms (txt, cmb), so implementing it into my VBA made sense - so I don't really want to get into that debate

-
There's also mixed opinion on accessors vs public variables. If I create a letter then I create a getter, except where I'm storing the return Cols in a collection

```
Option Explicit

Public Enum dataReturnType
arrayVals
collectionVals
dictionaryVals
End Enum

Public Enum sourceFileType
csv = 1 '
xls = 10 'old Excel
mdb 'old Access
xlsx = 100 'new Excel
xlsm 'new Excel macro-enabled

GetExternalData isn't ideal for a class name; if methods are verbs, classes/types are nouns. Class names should be nouns, not verbs.

When making a class, unless the class' primary concern is presentation, I leave MsgBox and other notifications out of it, and raise custom runtime errors instead.

This one caught my eye:

Property Let FileName(fName As String)
   strFileName = fName
   Select Case Right(strFileName, Len(strFileName) - InStr(strFileName, "."))
      Case Is = "txt", "csv", "lst"
         enumFileType = csv
      Case Is = "xls"
         enumFileType = xls
      Case Is = "xlsx", "xlsb"
         enumFileType = xlsx
      Case Is = "xlsm"
         enumFileType = xlsm
      Case Is = "mdb"
         enumFileType = mdb
      Case Is = "accdb"
         enumFileType = accdb
   End Select
End Property

If the extension is unknown, you'll have inconsistent internal state in your object. If the file name contains dots (e.g. some.file.with.dots.csv), there's a bug in your Case expression that will leave enumFileType unassigned. Consider using the much more robust FileSystemObject to retrieve a file's extension, instead of implementing your own:

Private Function GetFileExtension(ByVal path As String)
    With New Scripting.FileSystemObject
        GetFileExtension = .GetExtensionName(path)
    End With
End Function

Also, Case Is = is rather uncommon; this would be equivalent:

Property Let FileName(ByVal value As String)
    Dim extension As String
    extension = GetFileExtension(value)
    Select Case extension
        Case "txt", "csv", "lst"
            enumFileType = csv
        Case "xls"
            enumFileType = xls
        Case "xlsx", "xlsb"
            enumFileType = xlsx
        Case "xlsm"
            enumFileType = xlsm
        Case "mdb"
            enumFileType = mdb
        Case "accdb"
            enumFileType = accdb
        Case Else
            Err.Raise 5, TypeName(Me), "File format '." & extension & "' is not supported."
    End Select
    strFileName = value
End Property

Notice the Case Else that prevents assigning strFileName with a value that we don't know what to do with, by raising a runtime error.

The casing isn't consistent; in VBA public members should be PascalCase. I'm not going to say a word about Hungarian notation, except "ugh" (that's not a word, is it?).


There's also mixed opinion on accessors vs public variables.

Not really. A public field in a standard module is a global variable, and that's pretty much universally recognized as a bad practice whenever avoidable. A public field in a class module breaks encapsulation, which literally defeats the purpose of a class module. In every single object-oriented language, a public field is bad practice. How is VBA any different? Avoid public fields. Period.

What's up with the indentation in doConnectQT?

End With
         qT.Delete

Set doConnectQT = ws

End Function

The indentation of getConnectionProperties isn't consistent with the rest of the module either. I've copied your code into a class module and Rubberduck cleanly indented the whole thing in a split second.

Speaking of Rubberduck, here's what its code inspections have to say:

`Warning: Constant 'adOpenStatic' is implicitly Variant - CR14196VBA.ExternalData, line 134
Warning: Constant 'adLockOptimistic' is implicitly Variant - CR14196VBA.ExternalData, line 135
Warning: Constant 'adCmdText' is implicitly Variant - CR14196VBA.ExternalData, line 136
Suggestion: Public field 'WSname' breaks encapsulation - CR14196VBA.ExternalData, line 37
Suggestion: Public field 'WhereClause' breaks encapsulation - CR14196VBA.ExternalData, line 38
Suggestion: Public field 'RtnType' breaks encapsulation - CR14196VBA.ExternalData, line 39
Warning: Member 'Worksheets' implicitly references ActiveWorkbook - CR14196VBA.ExternalData, line 221
Hint: Member 'FilePath' is implicitly public - CR14196VBA.ExternalData, line 48
Hint: Member 'FilePath' is implicitly public - CR14196VBA.ExternalData, line 58
Hint: Member 'FileName' is implicitly public - CR14196VBA.ExternalData, line 62
Hint: Member 'FileName' is implicitly public - CR14196VBA.ExternalData, line 80
Hint: Member 'ReturnFields' is implicitly public - CR14196VBA.ExternalData, line 84
Hint: Member 'externalDataKeyColumn' is implicitly public - CR14196VBA.ExternalData, line 96
Hint: Member 'externalDataKeyColumn' is implicitly public - CR14196VBA.ExternalData, line 99
Suggestion: Move module-level variable 'strConnString' to a smaller scope. - CR14196VBA.ExternalData, line 26
Suggestion: Move module-level variable 'strExtendedProperties' to a smaller scope. - CR14196VBA.ExternalData, line 27
Suggestion: Move module-level variable 'rngOutput' to a smaller scope. - CR14196VBA.ExternalData, line 36
Suggestion: Move module-level variable 'WSname' to a smaller scope. - CR14196VBA.ExternalData, line 37
Suggestion: Move module-level variable 'WhereClause' to a smaller

StackExchange Code Review Q#142196, answer score: 3