modArraySupport: refactoring `IsArrayAllNumeric` and `IsVariantArrayNumeric`

Question

Foreword

I try to learn unit testing in (Excel) VBA using Rubberduck. As an example "project" I want to create tests for Chip Pearson's modArraySupport and afterwards refactor that module, because I think I already found some bugs. But because I am not a native English speaker, maybe I just get things wrong. So this might end in some questions that

1. need some clarification regarding the (real) purpose of the function when having a look at the functions description only and
2. a potential bug is really a bug and if my attempt to fix it is right.

(I already tried to contact Mr. Pearson via email directly, but didn't get an answer so far.)

---

Refactoring function(s)

To my opinion the two functions IsVariantArrayNumeric and IsArrayAllNumeric are almost identical. The only differences between these functions I could find are that

1. IsArrayAllNumeric has the optional argument to also allow for "numeric strings", and
2. IsVariantArrayNumeric also allows (sub-)arrays to be checked for numeric data.

In addition: Both functions seem to have a bug. While IsArrayAllNumeric directly loops through a single-dimensional array for the test of the elements

For Ndx = LBound(Arr) To UBound(Arr)

without testing if Arr is single-dimensional, IsVariantArrayNumeric checks the dimensionality, but for NumDims > 1 the stated test for the "multi-dimensional array" actually is only a test for a two-dimensional array

For Ndx = LBound(TestArray, DimNdx) To UBound(TestArray, DimNdx)

And unfortunately only the first element of a (sub-)array in IsVariantArrayNumeric is tested for numeric data.

So my suggestion would be to merge the two functions in IsArrayAllNumeric, add a second optional argument to allow also checking for (sub-)arrays, and of course to remove the above mentioned bugs.

'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
'IsArrayAllNumeric
'This function returns 'True' if 'Arr' is entirely numeric and 'False'
'otherwise. The 'AllowNumericStrings' parameter indicates whether strings
'containing numeric data are considered numeric. If this parameter is 'True', a
'numeric string is considered a numeric variable. If this parameter is omitted
'or 'False', a numeric string is not considered a numeric variable. Variants
'that are numeric or empty are allowed. Variants that are objects or
'non-numeric data are not allowed. With the 'AllowArrayElements' parameter it
'can be stated, if (sub-)arrays should also be tested for numeric data.
'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
Public Function IsArrayAllNumeric( _
   Arr As Variant, _
   Optional AllowNumericStrings As Boolean = False, _
   Optional AllowArrayElements As Boolean = False _
      ) As Boolean

   Dim element As Variant

   'Set the default return value
   IsArrayAllNumeric = False

   If Not IsArray(Arr) Then Exit Function
   If Not IsArrayAllocated(Arr) Then Exit Function

   'Loop through the array
   For Each element In Arr
      If IsObject(element) Then Exit Function

      Select Case VarType(element)
         Case vbEmpty
            'is (also) allowed
         Case vbString
            'For strings, check the 'AllowNumericStrings' parameter.
            'If True and the element is a numeric string, allow it.
            'If it is a non-numeric string, exit with 'False'.
            'If 'AllowNumericStrings' is 'False', all strings, even
            'numeric strings, will cause a result of 'False'.
            If AllowNumericStrings = True Then
               If Not IsNumeric(element) Then Exit Function
            Else
               Exit Function
            End If
         Case Is >= vbVariant
            'For Variants, disallow Objects.
            If IsObject(element) Then Exit Function
            'If the element is an array ...
            If IsArray(element) Then
               '... only test the elements, if (numeric) array elements are
               'allowed
               If AllowArrayElements Then
                  'Test the elements (recursively) with the same rules as the
                  'main array
                  If Not IsArrayAllNumeric( _
                        element, AllowNumericStrings, AllowArrayElements) Then _
                              Exit Function
               Else
                  Exit Function
               End If
            'If the element is not an array, test, if it is of numeric type.
            Else
               If Not IsNumeric(element) Then Exit Function
            End If
         Case Else
            If Not IsNumeric(element) Then Exit Function
      End Select
   Next

   IsArrayAllNumeric = True

End Function

So my questions and wishes are:

1. Did I get things right from the function descriptions?
2. If yes, are there better ways for an implementation?
3. If I should have introduced some other/new bugs, please show me where they are and how to fix them.

Unit tests

While modArraySupport is in an AddIn that is always loaded when Excel starts, I currently have the unit tests (module) in a separate Excel file. In this case the Excel file is called mod_Test_ArraySupport and the unit test module mod_Test_Array (I wanted to name it mod_Test_ArraySupport (as well) but then Rubberduck doesn't finish his refresh, so I guess there is some bug ...) with the code.

Option Explicit
Option Compare Text
Option Private Module

'@TestModule
'@Folder("Tests")

Private Assert As Rubberduck.PermissiveAssertClass
Private Fakes As Rubberduck.FakesProvider


'@ModuleInitialize
Public Sub ModuleInitialize()
   'this method runs once per module.
   Set Assert = New Rubberduck.PermissiveAssertClass
   Set Fakes = New Rubberduck.FakesProvider
End Sub

'@ModuleCleanup
Public Sub ModuleCleanup()
   'this method runs once per module.
   Set Assert = Nothing
   Set Fakes = Nothing
End Sub

'@TestInitialize
Public Sub TestInitialize()
   'this method runs before every test in the module.
End Sub

'@TestCleanup
Public Sub TestCleanup()
   'this method runs after every test in the module.
End Sub


'==============================================================================
'unit tests for 'IsArrayAllNumeric'
'==============================================================================

'@TestMethod
Public Sub IsArrayAllNumeric_NoArray_ReturnsFalse()
   On Error GoTo TestFail

   'Arrange:
   Dim V As Variant


   'Act:
   'Assert:
   Assert.IsFalse modArraySupport.IsArrayAllNumeric(V)

TestExit:
   Exit Sub
TestFail:
   Assert.Fail "Test raised an error: #" & Err.Number & " - " & Err.Description
End Sub


'@TestMethod
Public Sub IsArrayAllNumeric_UnallocatedArray_ReturnsFalse()
   On Error GoTo TestFail

   'Arrange:
   Dim V() As Variant


   'Act:
   'Assert:
   Assert.IsFalse modArraySupport.IsArrayAllNumeric(V)

TestExit:
   Exit Sub
TestFail:
   Assert.Fail "Test raised an error: #" & Err.Number & " - " & Err.Description
End Sub


'@TestMethod
Public Sub IsArrayAllNumeric_IncludingNumericStringAllowNumericStringsFalse_ReturnsTrue()
   On Error GoTo TestFail

   Dim V(1 To 3) As Variant


   'Arrange:
   V(1) = "100"
   V(2) = 2
   V(3) = Empty

   'Act:
   'Assert:
   Assert.IsFalse modArraySupport.IsArrayAllNumeric(V, False)

TestExit:
   Exit Sub
TestFail:
   Assert.Fail "Test raised an error: #" & Err.Number & " - " & Err.Description
End Sub


'@TestMethod
Public Sub IsArrayAllNumeric_IncludingNumericStringAllowNumericStringsTrue_ReturnsTrue()
   On Error GoTo TestFail

   Dim V(1 To 3) As Variant


   'Arrange:
   V(1) = "100"
   V(2) = 2
   V(3) = Empty

   'Act:
   'Assert:
   Assert.IsTrue modArraySupport.IsArrayAllNumeric(V, True)

TestExit:
   Exit Sub
TestFail:
   Assert.Fail "Test raised an error: #" & Err.Number & " - " & Err.Description
End Sub


'@TestMethod
Public Sub IsArrayAllNumeric_IncludingNonNumericString_ReturnsFalse()
   On Error GoTo TestFail

   Dim V(1 To 3) As Variant


   'Arrange:
   V(1) = "abc"
   V(2) = 2
   V(3) = Empty

   'Act:
   'Assert:
   Assert.IsFalse modArraySupport.IsArrayAllNumeric(V, True)

TestExit:
   Exit Sub
TestFail:
   Assert.Fail "Test raised an error: #" & Err.Number & " - " & Err.Description
End Sub


'@TestMethod
Public Sub IsArrayAllNumeric_Numeric1DVariantArray_ReturnsTrue()
   On Error GoTo TestFail

   Dim V(1 To 3) As Variant


   'Arrange:
   V(1) = 123
   V(2) = 456
   V(3) = 789

   'Act:
   'Assert:
   Assert.IsTrue modArraySupport.IsArrayAllNumeric(V)

TestExit:
   Exit Sub
TestFail:
   Assert.Fail "Test raised an error: #" & Err.Number & " - " & Err.Description
End Sub


'@TestMethod
Public Sub IsArrayAllNumeric_1DVariantArrayWithObject_ReturnsFalse()
   On Error GoTo TestFail

   Dim V(1 To 3) As Variant


   'Arrange:
   V(1) = 123
   Set V(2) = ThisWorkbook.Worksheets(1).Range("A1")
   V(3) = 789

   'Act:
   'Assert:
   Assert.IsFalse modArraySupport.IsArrayAllNumeric(V)

TestExit:
   Exit Sub
TestFail:
   Assert.Fail "Test raised an error: #" & Err.Number & " - " & Err.Description
End Sub


'@TestMethod
Public Sub IsArrayAllNumeric_1DVariantArrayWithUnallocatedEntry_ReturnsTrue()
   On Error GoTo TestFail

   Dim V(1 To 3) As Variant


   'Arrange:
   V(1) = 123
   V(3) = 789

   'Act:
   'Assert:
   Assert.IsTrue modArraySupport.IsArrayAllNumeric(V)

TestExit:
   Exit Sub
TestFail:
   Assert.Fail "Test raised an error: #" & Err.Number & " - " & Err.Description
End Sub


'@TestMethod
Public Sub IsArrayAllNumeric_Numeric2DVariantArray_ReturnsTrue()
   On Error GoTo TestFail

   Dim V(1 To 3, 4 To 5) As Variant


   'Arrange:
   V(1, 4) = 123
   V(2, 4) = 456
   V(3, 4) = 789

   V(1, 5) = -5
   V(3, 5) = -10

   'Act:
   'Assert:
   Assert.IsTrue modArraySupport.IsArrayAllNumeric(V)

TestExit:
   Exit Sub
TestFail:
   Assert.Fail "Test raised an error: #" & Err.Number & " - " & Err.Description
End Sub


'@TestMethod
Public Sub IsArrayAllNumeric_2DVariantArrayWithObject_ReturnsFalse()
   On Error GoTo TestFail

   Dim V(1 To 3, 4 To 5) As Variant


   'Arrange:
   V(1, 4) = 123
   Set V(2, 4) = ThisWorkbook.Worksheets(1).Range("A1")
   V(3, 4) = 789

   V(1, 5) = -5
   V(3, 5) = -10

   'Act:
   'Assert:
   Assert.IsFalse modArraySupport.IsArrayAllNumeric(V)

TestExit:
   Exit Sub
TestFail:
   Assert.Fail "Test raised an error: #" & Err.Number & " - " & Err.Description
End Sub


'@TestMethod
Public Sub IsArrayAllNumeric_1DVariantArrayWithArrayAllowArrayElementsFalse_ReturnsFalse()
   On Error GoTo TestFail

   Dim V(1 To 3) As Variant


   'Arrange:
   V(1) = 123
   V(2) = Array(-5)
   V(3) = 789

   'Act:
   'Assert:
   Assert.IsFalse modArraySupport.IsArrayAllNumeric(V)

TestExit:
   Exit Sub
TestFail:
   Assert.Fail "Test raised an error: #" & Err.Number & " - " & Err.Description
End Sub


'@TestMethod
Public Sub IsArrayAllNumeric_1DVariantArrayWithArrayAllowArrayElementsTrue_ReturnsTrue()
   On Error GoTo TestFail

   Dim V(1 To 3) As Variant


   'Arrange:
   V(1) = 123
   V(2) = Array(-5)
   V(3) = 789

   'Act:
   'Assert:
   Assert.IsTrue modArraySupport.IsArrayAllNumeric(V, , True)

TestExit:
   Exit Sub
TestFail:
   Assert.Fail "Test raised an error: #" & Err.Number & " - " & Err.Description
End Sub


'@TestMethod
Public Sub IsArrayAllNumeric_1DVariantArrayWithArrayAllowArrayElementsTrue_ReturnsFalse()
   On Error GoTo TestFail

   Dim V(1 To 3) As Variant


   'Arrange:
   V(1) = 123
   V(2) = Array(-5, "-5")
   V(3) = 789

   'Act:
   'Assert:
   Assert.IsFalse modArraySupport.IsArrayAllNumeric(V, , True)

TestExit:
   Exit Sub
TestFail:
   Assert.Fail "Test raised an error: #" & Err.Number & " - " & Err.Description
End Sub


'@TestMethod
Public Sub IsArrayAllNumeric_1DVariantArrayWithArrayAllowNumericStringsTrueAllowArrayElementsTrue_ReturnsTrue()
   On Error GoTo TestFail

   Dim V(1 To 3) As Variant


   'Arrange:
   V(1) = 123
   V(2) = Array(-5, "-5")
   V(3) = 789

   'Act:
   'Assert:
   Assert.IsTrue modArraySupport.IsArrayAllNumeric(V, True, True)

TestExit:
   Exit Sub
TestFail:
   Assert.Fail "Test raised an error: #" & Err.Number & " - " & Err.Description
End Sub

Interesting questions to answer here are, e.g.

• Is it the right decision to separate the unit test modules files from the code modules files? (I was thinking that is clever because then Rubberduck is much faster when refreshing. The above mentioned AddIn has a lot of modules with a lot of procedures/functions and already takes some seconds for a refresh without the unit tests)
• In the unit tests the function is called with modArraySupport.IsArrayAllNumeric instead of just IsArrayAllNumeric. That is because in the Excel file I also have the original modArraySupport module which I renamed to modArraySupport_old to quickly have a look at the original code and to easily switch the tests to the original module/code to see what happened there. What are your recommendations here? In the meantime I think that it will be cleverer to rename the refactored module to e.g. modArraySupport2 and leave the original modules name. Thus, it is clear that there are some breaking changes in the code. Do you agree?
• I am not sure if I have understood the naming convention of unit tests right. If you have better names for test, please let me know.
• Did I miss some tests or are there too many?

Answer 1

I'm not addressing the Unit Test stuff and we're missing the IsArrayAllocated function. Essentially I'm just talking about this part

For Each element In Arr
        If IsObject(element) Then Exit Function
        
        Select Case VarType(element)
        Case vbEmpty
        Case vbString
            If AllowNumericStrings = True Then
                If Not IsNumeric(element) Then Exit Function
            Else
                Exit Function
            End If
        Case Is >= vbVariant
            If IsObject(element) Then Exit Function
            If IsArray(element) Then
                If AllowArrayElements Then
                    If Not IsArrayAllNumeric( _
                       element, AllowNumericStrings, AllowArrayElements) Then _
                       Exit Function
                Else
                    Exit Function
                End If
            Else
                If Not IsNumeric(element) Then Exit Function
            End If
        Case Else
            If Not IsNumeric(element) Then Exit Function
        End Select
    Next
End Sub

---

You'll see that when you start the loop you check if the element is an object, but also do the same thing in Case Is >= vbVariant. No need to double that.

You also check if the element is numeric three times.

For Case vbString, you have

If AllowNumericStrings = True Then

But all you need is

If AllowNumericStrings Then

Clarification

In the image you'll see

1. Lines 10 and 24 do the same test
2. Lines 38 and 41 do the same test
3. Line 14 doesn't do anything, as in it's not clear what it should do
4. You have 10 exit points. And Two of them are just Else Exit Function

Boiled down is -

Public Function IsArrayAllNumeric(Arr As Variant, Optional AllowNumericStrings As Boolean = False, Optional AllowArrayElements As Boolean = False) As Boolean

   Dim element As Variant

   If Not IsArray(Arr) Or Not IsArrayAllocated(Arr) Then Exit Function
   For Each element In Arr
      If IsObject(element) Then Exit Function

      Select Case VarType(element)
         Case vbEmpty
         Case vbString
            If AllowNumericStrings And Not IsNumeric(element) Then Exit Function
            If Not AllowNumericStrings Then Exit Function
         Case Is >= vbVariant

            If IsArray(element) And AllowArrayElements And Not IsArrayAllNumeric(element, AllowNumericStrings, AllowArrayElements) Then Exit Function
            If Not IsArray(element) Then Exit Function
         Case Else
            If Not IsNumeric(element) Then Exit Function
      
      End Select
   Next
   IsArrayAllNumeric = True
End Function

Right, now these are your cases

     Case vbEmpty
     Case vbString
        If AllowNumericStrings And IsNumeric(element) Then Exit Function

     Case Is >= vbVariant
        
        If IsArray(element) And AllowArrayElements And Not IsArrayAllNumeric(element, AllowNumericStrings, AllowArrayElements) Then Exit Function
        If Not IsArray(element) Then Exit Function
     
     Case Else
        If Not IsNumeric(element) Then Exit Function

Which cane be boiled down again

For Each element In Arr
        If IsObject(element) Then Exit Function
        If VarType(element) = vbEmpty Then GoTo NextFor
        If VarType(element) = vbString And AllowNumericStrings And IsNumeric(element) Then Exit Sub
        If VarType(element) >= vbVariant Then
            If IsArray(element) And AllowArrayElements And Not IsArrayAllNumeric(element, AllowNumericStrings, AllowArrayElements) Then Exit Function
            If Not IsArray(element) Then Exit Function
        End If
        If VarType(element) <> vbString And VarType(element) >= vbVariant And Not IsNumeric(element) Then Exit Function
NextFor:
   Next

I show you this not because I want you to do it this way, but to showcase how many of your arguments are being duplicated.

And you'll see your comments are gone - "code tell you how, comments tell you why". The code should speak for itself, if it needs a comment, it might need to be made more clear. If not, the comment should describe why you're doing something rather than how you're doing it. Here are a few reasons to avoid comments all together.