I see several issues with this.
First is the naming. Row
and Column
both hide global Excel variable (although whether this is good or bad is debatable...). This otherwise valid code becomes a compile error "Expected Array" everywhere in the project that contains the enumeration.
Public Sub Test()
Debug.Print Row(1).Address
End Sub
I would personally change the Enum
name to something like LimitDimension
(Pascal case for namespace identifiers) and the two options to LimitRow
and LimitColumn
. This will be much less likely to create collisions. Note that r
and c
are also really common local loop counter variables. If they're public enumeration member, they pollute the global namespace and can create bizarre\misleading compile errors:
Option Explicit
Public Sub Test1()
Dim c As Long
Debug.Print c 'This works just fine, prints 0.
End Sub
Public Sub Test2()
Debug.Print c
Dim c As Long 'Compile error. Duplicate declaration in same scope.
End Sub
Public Sub Test3()
For c = 1 To 5 'Compile error. Variable required - can't assign to this expression.
Debug.Print c
Next
End Sub
Public Sub Test4()
Dim c As Long
For c = 1 To 5 'Works fine.
Debug.Print c
Next
End Sub
Second, enumerations should only have one member for each distinct value. The only real point to using them in VBA is to make your code more readable (there's no strict type enforcement), and it isn't immediately clear when you see something like this...
Select Case foo
Case rowOrColumnOption.r
'Do something.
Case rowOrColumnOption.Row
'Do something else.
End Select
...that it's a coding error. This is immediately obvious:
Select Case foo
Case rowOrColumnOption.Row
'Do something.
Case rowOrColumnOption.Row
'Do something else.
End Select
Third (on a related note), you should use the Enum
explicitly when you're checking for it's members. This...
If rowOrColumn = 0 Then
'...
ElseIf rowOrColumn = 1 Then
'...
Else
'...
End If
...is much clearer as:
If rowOrColumn = rowOrColumnOption.Row Then
'...
ElseIf rowOrColumn = rowOrColumnOption.Column Then
'...
Else
'...
End If
Not only is the much clearer as to the intent of the code, it also prevents it from breaking if you decide to or need to change the values of the members. It also makes the code document itself, so you don't have to sprinkle comments like this around:
' 0 corresponds to "Row" in the custom variable type "rowOrColumnOption".
Fourth, IMHO the statement delimiter :
is if not pure evil, mostly evil. It's much more natural to read code with the statements arranged vertically than it is if statements are arranged horizontally. This is much easier to process at a glance:
Public Enum rowOrColumnOption
r = 0
Row = 0
c = 1
Column = 1
End Enum
While the intent may have been to group aliased members, you can make this explicit by just assigning aliases within the declaration itself:
Public Enum rowOrColumnOption
Row = 0
r = Row
Column = 1
c = Column
End Enum
Finally, just as an aside - I have no idea what ...some code...
is doing in GetHighestRowOrCol
, but from the If
block it appears that ...some code...
is calculating both maxRow
and maxCol
when it only ever returns one of them. I'm guessing this would be a more efficient structure:
If rowOrColumn = rowOrColumnOption.Row Then
'...some code to determine the highest row number.
GetHighestRowOrCol = maxRow
ElseIf rowOrColumn = rowOrColumnOption.Column Then
'...some code to determine the highest column number.
GetHighestRowOrCol = maxCol
Else
GetHighestRowOrCol = 0
End If