# modArraySupport: refactoring IsArrayAllNumeric and IsVariantArrayNumeric

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?
• 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 :-) Jan 8, 2018 at 19:17
• 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. Jan 8, 2018 at 19:29
• @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. Jan 8, 2018 at 19:48
• @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 ;) Jan 8, 2018 at 19:50
• 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... Mar 17, 2018 at 0:16

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.

• 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.) ... Mar 16, 2018 at 17:56
• ... 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? Mar 16, 2018 at 18:10
• I added some clarification. I don't see any isValid in there. Mar 16, 2018 at 22:49
• (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. Mar 18, 2018 at 19:31
• 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? Mar 18, 2018 at 19:32