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?

• Mathieu Guindon: Is it the right decision to separate the unit test modules from the code modules? - absolutely. Rubberduck uses the @TestModule annotation to know what modules to look for @TestMethod procedures in (it doesn't look for tests in other modules), so having actual code in a test module would be rather messy :-)
• Mathieu Guindon: About "I wanted to name it mod_TestArraySupport but then Rubberduck gives finish his refresh, so I guess there is some bug" - feel free to join the devs in VBA Rubberducking chat, and/or to create a new issue on the Rubberduck repository.
• Stefan Pinnow: @Mat'sMug, sorry, I used the wrong wording: I meant to separate code files from unit test files instead of modules, i.e. I have an Excel AddIn file with the code modules and several Excel files, each containing on unit test module for a code module.
• Stefan Pinnow: @Mat'sMug, I will file an issue in the tracker, when I have further investigated that issue. Currently this "bug" is too vague to me to dare an issue ;)
• Vogel612: removed the [rubberduck]-tag, as this question is not actually related to developing rubberduck itself, which is the scope of the tag. Might be useful to put that idea up for debate, though...

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

enter image description here

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.

• Stefan Pinnow: Many thanks for your answer. The IsArrayAllocated function is another function in the modArraySupport module mentioned in the foreword of my question and can be copied from there. Then you can add the line If Not IsArrayAllocated(Arr) Then Exit Function after If Not IsArray(Arr) Then Exit Function again. Then it seems that you missed to add the optional arguments of IsArrayAllNumeric also to IsValid, right? (At least I get a compiler error when they are not present.) ...
• Stefan Pinnow: ... After applying the above changes I ran my unit tests and 3 failed. That are namely IsArrayAllNumeric_IncludingNumericStringAllowNumericStringsFalse_ReturnsFalse, IsArrayAllNumeric_1DVariantArrayWithArrayAllowNumericStringsTrueAllowArrayElementsTrue_ReturnsTrue, and IsArrayAllNumeric_1DVariantArrayWithArrayAllowArrayElementsTrue_ReturnsTrue. So your approach doesn't seem to work when there is a numeric string but AllowNumericStrings = False, and the other two I cannot simply summarize. Could you have a look at that again?
• Raystafarian: I added some clarification. I don't see any isValid in there.
• Stefan Pinnow: (IsValid was in your first (unedited) answer. Also with your current answer (revision 7) I got 5 failing unit tests, but I think I have understood your point and could modify your approach to work.
• Stefan Pinnow: Because I am new at CodeReview, shall I add the reviewed code as an edit to my question or as a Self-Answer so others know the "final" code as well?