6
\$\begingroup\$

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:

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

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

  3. 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

  4. 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
 accdb 'new Access
End Enum
Private Enum getDataMethod
 ADO_Record_Set
 Web_QT
End Enum
Private strFilePath As String
Private strFileName As String
Private enumFileType As sourceFileType
Private strConnString As String
Private strExtendedProperties As String
Private strSelectClause As String
Private strFromClause As String
Private colReturnFields As Collection
Private enumRetrievalMethod As getDataMethod
Private strFormatString As String
Private boolIMEX As Boolean
Private boolHDR As Boolean
Private intkeyCol As Integer
Private rngOutput as Range
Public WSname As String
Public WhereClause As String
Public RtnType As dataReturnType
Private Sub Class_Initialize()
 boolHDR = True
 boolIMEX = True
 Set colReturnFields = New Collection
 strSelectClause = "*"
End Sub
Property Let FilePath(fullPathOnly As String)
 If Left(fullPathOnly, 4) = "http" Then
 enumRetrievalMethod = Web_QT
 If Right(fullPathOnly, 1) = "/" Then strFilePath = fullPathOnly Else strFilePath = fullPathOnly & "/"
 Else
 enumRetrievalMethod = ADO_Record_Set
 If Right(fullPathOnly, 1) = "\" Then strFilePath = fullPathOnly Else strFilePath = fullPathOnly & "\"
 End If
End Property
Property Get FilePath() As String
 FilePath = strFilePath
End Property
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
Property Get FileName() As String
 FileName = strFileName
End Property
Property Let ReturnFields(colNums As Variant)
 Dim colNum As Variant
 Select Case VarType(colNums)
 Case Is < vbVariant
 colReturnFields.Add colNums
 Case Else
 For Each colNum In colNums
 colReturnFields.Add colNum
 Next
 End Select
End Property
Property Let externalDataKeyColumn(colNum As Integer)
 intkeyCol = colNum
End Property
Property Get externalDataKeyColumn() As Integer
 externalDataKeyColumn = intkeyCol
End Property
Public Function getExternalData() As Variant
 Dim ReturnVals As Object
 Dim DataProcessor As Object
 Set DataProcessor = New DataProcessor
 Select Case enumRetrievalMethod
 Case Is = getDataMethod.ADO_Record_Set
 Set ReturnVals = doConnectRS
 Select Case Me.RtnType
 Case Is = dataReturnType.arrayVals
 getExternalData = DataProcessor.collection2Array(ReturnVals, Me.externalDataKeyColumn, True)
 Case Is = collectionVals
 Set getExternalData = ReturnVals
 Case Is = dictionaryVals
 Set getExternalData = New scripting.Dictionary
 Set getExternalData = DataProcessor.collection2Dictionary(ReturnVals, Me.externalDataKeyColumn)
 End Select
 Case Is = getDataMethod.Web_QT
 Set ReturnVals = doConnectQT
 Select Case Me.RtnType
 Case Is = dataReturnType.arrayVals
 getExternalData = ReturnVals.UsedRange
 Case Is = collectionVals
 Set getExternalData = DataProcessor.array2Collection(ReturnVals.UsedRange, Me.externalDataKeyColumn)
 Case Is = dictionaryVals
 Set getExternalData = DataProcessor.array2Dictionary(ReturnVals.UsedRange, Me.externalDataKeyColumn)
 End Select
 End Select
End Function
Private Function doConnectRS() As Collection
 'RecordSet Constants
 Const adOpenStatic = 3
 Const adLockOptimistic = 3
 Const adCmdText = &H1
 Dim dp As New DataProcessor
 Dim objRecordSet As Object, objConnection As Object
 Dim varRS As Variant
 Dim connString As String, SqlStatement As String
 Dim colNum As Integer, rowNum As Integer
 Dim colRS As New Collection
 Dim keyCol As Variant
 If Not compatibleSource Then Exit Function
 connString = getConnectionProperties
 SqlStatement = "Select " & strSelectClause & " FROM " & strFromClause
 If WhereClause <> vbNullString Then SqlStatement = SqlStatement & " WHERE " & WhereClause
 Set objConnection = CreateObject("ADODB.Connection")
 Set objRecordSet = CreateObject("ADODB.Recordset")
 objConnection.Open connString
 objRecordSet.Open SqlStatement, objConnection, adOpenStatic, adLockOptimistic, adCmdText
 If colReturnFields.Item(1) = "all" Then
 colReturnFields.Remove (1)
 For colNum = 1 To objRecordSet.Fields.Count
 colReturnFields.Add colNum
 Next
 End If
 Do Until objRecordSet.EOF
 ReDim varRS(1 To colReturnFields.Count)
 For colNum = 1 To colReturnFields.Count
 If IsNull(objRecordSet.Fields.Item(CLng(colReturnFields.Item(colNum) - 1))) Then
 varRS(colNum) = vbNullString 'If the SQL statement returns Null populate with vbNullString rather than Null
 Else
 varRS(colNum) = objRecordSet.Fields.Item(CLng(colReturnFields.Item(colNum) - 1))
 End If
 Next
 colRS.Add varRS
 objRecordSet.MoveNext
 Loop
 Set objRecordSet = Nothing
 Set objConnection = Nothing
 Set doConnectRS = colRS
End Function
Private Function compatibleSource() As Boolean
 Select Case enumFileType
 Case Is > xlsx
 If Application.Version < 12 Then
 MsgBox "Incompatible source file selected!", vbCritical
 compatibleSource = False
 Else
 compatibleSource = True
 End If
 Case Else
 compatibleSource = True
 End Select
End Function
Private Function getConnectionProperties()
Dim HDR As String
If boolHDR Then HDR = "HDR=YES" Else HDR = "HDR=NO"
Select Case enumFileType
 Case Is = xls
 strConnString = "Provider=Microsoft.Jet.OLEDB.4.0;Data Source=" & strFilePath & strFileName & ";"
 strExtendedProperties = "Extended Properties=""Excel 8.0;"
 strFromClause = "[" & WSname & "$]"
 Case Is = csv
 strConnString = "Provider=Microsoft.Jet.OLEDB.4.0;Data Source=" & strFilePath & ";"
 strExtendedProperties = "Extended Properties=""text; FMT=Delimited;"
 strFromClause = strFileName
 Case Is = xlsx
 strConnString = "Provider=Microsoft.ACE.OLEDB.12.0;Data Source=" & strFilePath & strFileName & ";"
 strExtendedProperties = "Extended Properties=""Excel 12.0 Xml;"
 strFromClause = "[" & WSname & "$]"
 Case Is = xlsm
 strConnString = "Provider=Microsoft.ACE.OLEDB.12.0;Data Source=" & strFilePath & strFileName & ";"
 strExtendedProperties = "Extended Properties=""Excel 12.0 Macro;"
 strFromClause = "[" & WSname & "$]"
End Select
getConnectionProperties = strConnString & strExtendedProperties & HDR & ";"
If boolIMEX Then getConnectionProperties = getConnectionProperties & "IMEX=1;"
getConnectionProperties = getConnectionProperties & """"
End Function
Private Function doConnectQT() As Worksheet
Dim ws As Worksheet
Dim qT As QueryTable
If rngOutput Is Nothing Then
 Set ws = Worksheets.Add
 Set rngOutput = ws.Range("a1")
Else
 Set ws = rngOutput.Parent
End If
myFileName = "URL; " & Me.FilePath & Me.FileName
 Set qT = ws.QueryTables.Add(Connection:=myFileName, Destination:=rngOutput)
 With qT
 .Name = "myQT"
 .fieldNames = True
 .RowNumbers = False
 .FillAdjacentFormulas = False
 .PreserveFormatting = True
 .RefreshOnFileOpen = False
 .BackgroundQuery = True
 .RefreshStyle = xlInsertDeleteCells
 .SavePassword = False
 .SaveData = True
 .AdjustColumnWidth = True
 .RefreshPeriod = 0
 .WebSelectionType = xlEntirePage
 .WebFormatting = xlWebFormattingNone
 .WebPreFormattedTextToColumns = True
 .WebConsecutiveDelimitersAsOne = True
 .WebSingleBlockTextImport = False
 .WebDisableDateRecognition = False
 .WebDisableRedirections = False
 .Refresh BackgroundQuery:=False
 End With
 qT.Delete
Set doConnectQT = ws
End Function

For the sake of completeness, I will also post the 'DataProcessor' class which I use to convert the returned collection to a dictionary or array if needed

Public Function collection2Array(colIn As Collection, Optional ByVal keyCol As Integer, Optional writeOut As Boolean) As Variant
 Dim rowCount As Long, colCount As Long
 Dim arrOut As Variant
 Select Case VarType(colIn.Item(1))
 Case Is >= vbVariant 'if the collection contains arrays
 ReDim arrOut(1 To colIn.Count, 1 To UBound(colIn.Item(1))))
 For rowCount = 1 To UBound(colIn.Item(1))
 For colCount = 1 To UBound(arrOut, 2)
 arrOut(rowCount, colCount) = colIn.Item(rowCount)(colCount)
 Next
 Next
 Case Else
 If writeOut Then 'we'll return a 2D array with 1 column suitable for writing directly to a worksheet
 ReDim arrOut(1 To colIn.Count, 1 To 1)
 For rowCount = 1 To colIn.Count
 arrOut(rowCount, 1) = colIn.Item(rowCount)
 Next
 Else
 ReDim arrOut(1 To colIn.Count)
 For rowCount = 1 To colIn.Count
 arrOut(rowCount) = colIn.Item(rowCount)
 Next
 End If
 End Select
 collection2Array = arrOut
End Function
Public Function collection2Dictionary(colIn As Collection, Optional ByVal keyCol As Integer, Optional compareMode As VbCompareMethod = vbTextCompare) As scripting.Dictionary
 Dim colVal As Variant
 Set collection2Dictionary = New scripting.Dictionary
 collection2Dictionary.compareMode = compareMode
 If keyCol = 0 Then keyCol = 1
 For Each colVal In colIn
 collection2Dictionary.Item(colVal(keyCol)) = colVal
 Next
End Function
asked Sep 23, 2016 at 3:57
\$\endgroup\$
4
  • 3
    \$\begingroup\$ Just a note, the reason people dislike hungarian is because the prefixes are too generic. Knowing that someting is, say, an Integer is not worth the clutter. Knowing that something is an array index, though, or a combo box, that's more useful. I recommend the excellent Joel Spolsky post on the topic: joelonsoftware.com/articles/Wrong.html \$\endgroup\$ Commented Sep 23, 2016 at 8:42
  • 3
    \$\begingroup\$ One suggestion for you as an upgrade someday: implement each of the data types (csv, xls, accb, etc...) as its own class, but have them implement the same interface (you might call it IDataSource or some other suitable thing). That way, the logics for each different source are loosely coupled from the others, making your code more robust and easier to update (if the xls code isn't working, you won't risk making an update that might break the others). Also, you can make a "factory" method that returns an IDataSource that can tell by the file name which actual class to instantiate. \$\endgroup\$ Commented Sep 23, 2016 at 15:09
  • 1
    \$\begingroup\$ @Kaz - I've seen that article. The VBA developer's handbook recommends Hungarian, and I find it useful. For example, the Dictionary and Collection have the same methods, so context doesn't tell me which is which, but the prefix does. And I work for a transport company, where we have "Connote Numbers" and "Run Numbers" which can actually have alpha characters, so strConnoteNumber and strRunNo leave no ambiguity as to what data-type they actually are \$\endgroup\$ Commented Sep 24, 2016 at 2:34
  • 1
    \$\begingroup\$ @Blackhawk - That's actually a really good idea! Thank you very much, that had never occurred to me, and is exactly the type of feedback I was hoping for \$\endgroup\$ Commented Sep 24, 2016 at 2:35

1 Answer 1

3
\$\begingroup\$

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 scope. - CR14196VBA.ExternalData, line 38
Suggestion: Move module-level variable 'RtnType' to a smaller scope. - CR14196VBA.ExternalData, line 39
Warning: Instruction contains multiple declarations - CR14196VBA.ExternalData, line 138
Warning: Instruction contains multiple declarations - CR14196VBA.ExternalData, line 140
Warning: Instruction contains multiple declarations - CR14196VBA.ExternalData, line 141
Suggestion: Consider renaming enum member 'csv' - CR14196VBA.ExternalData, line 10
Suggestion: Consider renaming enum member 'xls' - CR14196VBA.ExternalData, line 11
Suggestion: Consider renaming enum member 'mdb' - CR14196VBA.ExternalData, line 12
Suggestion: Consider renaming enum member 'xlsx' - CR14196VBA.ExternalData, line 13
Suggestion: Consider renaming enum member 'xlsm' - CR14196VBA.ExternalData, line 14
Suggestion: Consider renaming variable 'dp' - CR14196VBA.ExternalData, line 137
Suggestion: Consider renaming variable 'HDR' - CR14196VBA.ExternalData, line 190
Suggestion: Consider renaming variable 'ws' - CR14196VBA.ExternalData, line 217
Suggestion: Consider renaming variable 'qT' - CR14196VBA.ExternalData, line 218
Warning: Return value of function 'getExternalData' is never used. - CR14196VBA.ExternalData, line 103
Hint: Parameter 'fullPathOnly' is implicitly passed by reference - CR14196VBA.ExternalData, line 48
Hint: Parameter 'fName' is implicitly passed by reference - CR14196VBA.ExternalData, line 62
Hint: Parameter 'colNums' is implicitly passed by reference - CR14196VBA.ExternalData, line 84
Hint: Parameter 'colNum' is implicitly passed by reference - CR14196VBA.ExternalData, line 96
Hint: Return type of member 'getConnectionProperties' is implicitly 'Variant' - CR14196VBA.ExternalData, line 189
Suggestion: Parameter 'fullPathOnly' can be passed by value - CR14196VBA.ExternalData, line 48
Suggestion: Parameter 'fName' can be passed by value - CR14196VBA.ExternalData, line 62
Suggestion: Parameter 'colNums' can be passed by value - CR14196VBA.ExternalData, line 84
Suggestion: Parameter 'colNum' can be passed by value - CR14196VBA.ExternalData, line 96
Warning: function 'getExternalData' is not used - CR14196VBA.ExternalData, line 103
Hint: Object reference 'dp' is self-assigned - CR14196VBA.ExternalData, line 137
Hint: Object reference 'colRS' is self-assigned - CR14196VBA.ExternalData, line 142
Warning: Variable 'strFormatString' is never assigned - CR14196VBA.ExternalData, line 32
Warning: Variable 'WSname' is never assigned - CR14196VBA.ExternalData, line 37
Warning: Variable 'WhereClause' is never assigned - CR14196VBA.ExternalData, line 38
Warning: Variable 'RtnType' is never assigned - CR14196VBA.ExternalData, line 39
Warning: Variable 'rowNum' is never assigned - CR14196VBA.ExternalData, line 141
Warning: Variable 'keyCol' is never assigned - CR14196VBA.ExternalData, line 143
Warning: variable 'strFormatString' is not used - CR14196VBA.ExternalData, line 32
Warning: variable 'dp' is not used - CR14196VBA.ExternalData, line 137
Warning: variable 'rowNum' is not used - CR14196VBA.ExternalData, line 141
Warning: variable 'keyCol' is not used - CR14196VBA.ExternalData, line 143
Suggestion: Property 'ReturnFields' has no getter - CR14196VBA.ExternalData, line 84

Now, some of these inspection results are popping up only because I have no client code to consume the class, call its members and assign its public fields, but ignoring that, you can see that there are a number of serious issues with this code - variables declared but never used, ReturnFields property being write-only (consider making it a method instead), members are implicitly Public, parameters are implicitly passed ByRef when they could very well be passed ByVal, some variables have cryptic 2-3 letter names, etc.


I'm not sure I like how you're passing arbitrary concatenated SQL to an ADODB connection - the WHERE clause should already be part of the query, and parameters should be ? question marks, and your code would be taking in actual parameter values (say, using a ParamArray parameter, or an array of values) and creating ADODB Parameter objects and executing an ADODB Command instead of concatenating arbitrary SQL.

</SqlSecurityRant>

I think replacing Null values with vbNullString should be some setting that the client code can decide about: "no value" is semantically very different from "empty value".

I'm not sure why you're late-binding ADODB here. You lose IntelliSense, take a performance hit at runtime, and you wouldn't need to redefine the adEnum constants.


In getConnectionProperties you're using the function's return value as if it were a local variable:

getConnectionProperties = strConnString & strExtendedProperties & HDR & ";"
If boolIMEX Then getConnectionProperties = getConnectionProperties & "IMEX=1;"
getConnectionProperties = getConnectionProperties & """"

Avoid this, it hurts readability and looks like a recursive call. And declare the function As String - right now it's implicitly returning a Variant - there's so much Variant and Object in this code, you have implicit type conversions through the roof!


I'd like to point out this inspection result:

Hint: Object reference 'colRS' is self-assigned - CR14196VBA.ExternalData, line 142

Consider this code:

Public Sub DoSomething()
 Dim foo As New Collection
 foo.Add "test1"
 Set foo = Nothing
 foo.Add "test2" ' runtime error? isn't foo supposed to be Nothing?
End Sub

When using As New at procedure scope like you do here:

Dim colRS As New Collection

...you're changing how VBA treats that object's lifetime, in a way that can introduce subtle and surprising bugs. Avoid As New at procedure scope.


It's hard to tell module-scope variables from globals. Adepts of Hungarian notation usually annotate their private fields with some m_ prefix for that purpose. I have a better way (granted, that T prefix is totally Hungarian - as is the I prefix I use on all my interface classes... Hungarian does have a number of benefits... just not in variable naming):

Private Type TClassName
 Foo As String
 Bar As Integer
 '...
End Type
Private this As TClassName
Public Property Get Foo() As String
 Foo = this.Foo
End Property
Public Property Let Foo(ByVal value As String)
 this.Foo = value
End Property
Public Property Get Bar() As Integer
 Bar = this.Bar
End Property
Public Property Let Bar(ByVal value As Integer)
 this.Bar = value
End Property
'...

By wrapping all your private fields in a Private Type, you make it idiot-proof what the class' fields are, and you have one single field (this) to refer to everywhere in the class - that way whenever you read the code and see anything prefixed with this., you know you can only be looking at a private field.

It also eliminates the casing/naming problem: the properties and their corresponding underlying private field names can match without breaking any naming convention, and without having to resort to funky m_ prefixes.

As for the relevance of Hungarian notation (sorry!), when your IDE tells you what the type of a variable is, what purpose does the prefix serve?

colReturnFields (variable: Collection

Meaningful names and consistent naming conventions are much more useful than Hungarian notation: I know I'm looking at a collection, or group of things, simply because Fields is plural - the col prefix isn't really useful at all.

enumRetrievalMethod (variable: getDataMethod

If I wasn't paying attention, I'd think getDataMethod is a Function or some member/procedure; the enum type name should simply not start with a verb - it's a type, therefore it's a noun.

I guess that's a wrap :)

(also: avoid underscores in identifier names - spare your future self headaches, stick to PascalCase for types and their members, and camelCase for locals and parameters)

answered Sep 23, 2016 at 19:24
\$\endgroup\$
7
  • 1
    \$\begingroup\$ Thanks for the comment. You're entirely right about having a CASE ELSE for unknown filetypes, and I'm pleased to learn about Scripting.FileSystemObject for retrieving file extensions, much better than finding the period in the name! As for your code inspections, I'm not sure what to make of most of that. Could you provide me with some info on your this technique? I don't immediately see the benefit. As for Hungarian, "when your IDE tells you what the type of a variable is [in break mode]", it can be useful to know at design-time, too! "strConnoteNumber" leaves no doubt that it's alphanumeric \$\endgroup\$ Commented Sep 24, 2016 at 3:03
  • \$\begingroup\$ "I know I'm looking at a collection, or group of things, simply because Fields is plural - the col prefix isn't really useful at all" - it's useful to me to know whether the data-type is a Dictionary, or a Collection, or an Array (since collections and dictionaries have the same methods, context doesn't help). And you're right about the MsgBox - that's a hold-over from some testing, and should have already been removed! \$\endgroup\$ Commented Sep 24, 2016 at 3:15
  • 1
    \$\begingroup\$ Try Ctrl+i on any variable at design time... these RD screenshots are at design time, too. As for the inspections, they identify unused variables, parameters passed ByRef that can/should be ByVal, members that are implicitly Public that could use an explicit access modifier, things declared without an explicit type that are implicitly Variant, etc. The benefits of the this field are already in the answer - you'll never clash with a global variable if you do that, and it's easier to tell a field from a local or parameter too. Not to mention instant serialization if you need it. \$\endgroup\$ Commented Sep 24, 2016 at 13:24
  • 1
    \$\begingroup\$ I know this is not a forum, but this is a great opportunity for me to actually learn. It was this article on Stack Overflow that convinced me accessors are unnecessary if all you're doing is storing and returning in a 'dumb container': stackoverflow.com/questions/1568091/why-use-getters-and-setters. I don't see that Ctrl+i is more convenient than col*. Finally, my intention is to move everything to late binding, as my organisation has Excel from XP to 2010 still in use (which is why I do the version validation in the class) \$\endgroup\$ Commented Sep 24, 2016 at 23:53
  • 1
    \$\begingroup\$ Odd, that SO page is presenting overwhelming evidence and explanation about why public fields are a bad idea and why exposing class data as properties is better. If that won't convince you, I don't know what will. Your class is not a "dumb data holder" (that's exactly what a Public Type is; a structure exposing a bunch of public fields and nothing more), it embeds functionality and presents an interface - arguably a number of these fields should simply not be exposed at all. \$\endgroup\$ Commented Sep 25, 2016 at 0:01

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.