trjhtr

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 at the 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 dat.

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?

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.