Limit acceptable values for function parameter in VBA

Question

I wrote a function in VBA for Excel that finds the highest row and highest column number in a range, which may include multiple, non-contiguous areas.

The function contains a parameter I call rowOrColumnOptionand I want to only allow values r or c (to return either a row or column number).

I was able to do this with an enumerated variable type:

Public Enum rowOrColumnOption
 ' Custom variable type, used to limit allowed values in a
 ' function parameter to either r (row) or "c" (column).
 r = 0: Row = 0
 c = 1: Column = 1
End Enum

I then assigned this variable type to a parameter in my function:

Public Function GetHighestRowOrCol(ByVal targetRange As Range, ByVal rowOrColumn As rowOrColumnOption) As Long
...some code...
If rowOrColumn = 0 Then
 ' Return the highest row number.
 ' 0 corresponds to "Row" in the custom variable type "rowOrColumnOption".
 GetHighestRowOrCol = maxRow
ElseIf rowOrColumn = 1 Then
 ' Return the highest column number.
 ' 1 corresponds to "Column" in the custom variable type "rowOrColumnOption".
 GetHighestRowOrCol = maxCol
Else
 ' Return an error value of zero.
 GetHighestRowOrCol = 0
End If
End Function

While this does work, I wonder if there is a better, simpler, faster, clearer, smarter way to do this, without using Enum. I am also open to any general critique/advice about the code overall.

Answer 1

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

• ChrisB: Great suggestions @Comintern! I made changes based on all of them, including changing the flow of some code to calculate either the row or the column number rather than both.