3
\$\begingroup\$

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?
\$\endgroup\$
  • \$\begingroup\$ 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 :-) \$\endgroup\$ – Mathieu Guindon Jan 8 '18 at 19:17
  • \$\begingroup\$ 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. \$\endgroup\$ – Mathieu Guindon Jan 8 '18 at 19:29
  • \$\begingroup\$ @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. \$\endgroup\$ – Stefan Pinnow Jan 8 '18 at 19:48
  • \$\begingroup\$ @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 ;) \$\endgroup\$ – Stefan Pinnow Jan 8 '18 at 19:50
  • \$\begingroup\$ 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... \$\endgroup\$ – Vogel612 Mar 17 '18 at 0:16
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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.) ... \$\endgroup\$ – Stefan Pinnow Mar 16 '18 at 17:56
  • \$\begingroup\$ ... 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? \$\endgroup\$ – Stefan Pinnow Mar 16 '18 at 18:10
  • \$\begingroup\$ I added some clarification. I don't see any isValid in there. \$\endgroup\$ – Raystafarian Mar 16 '18 at 22:49
  • \$\begingroup\$ (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. \$\endgroup\$ – Stefan Pinnow Mar 18 '18 at 19:31
  • \$\begingroup\$ 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? \$\endgroup\$ – Stefan Pinnow Mar 18 '18 at 19:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.