2
\$\begingroup\$

Coming from a Java/Scala background with a strong focus on OOP+FP, I recently started working with Excel+VBA to implement a front-end for a SaaS. It was like stepping way back in time, over two decades, in software engineering trying to understand how to use and exploit VBA.

In the ensuing 3 months of being immersed in VBA, I have ended up running into and then unique solving a number of problems using a more principled software engineering principled approach. This includes things like DRY (Don't Repeat Yourself which means eschew copy/pasta), DbC (Design by Contract which means define clear boundaries around types and functions), encapsulation, immutability, referential transparency, etc.

The first things I hit had to do with VBA's Array. The code smell of the imperative solutions I found on StackOverflow and the web, in general, was very frustrating. Additionally, it seemed all the proposed solutions I found were not only not DRY (Don't Repeat Yourself) whatsoever, almost all of them seemed to have subtle errors for which they didn't account. After hitting issue after issue with just trying to figure out if an Array was allocated, empty, or defined (non-empty), I finally set about trying to create an optimal solution.

Given my lack of VBA-specific experience (I have a year's worth of VBA 3.0 from back in 1994-1995), I'd like to understand to what degree the solution I am proposing is safe, robust, and performant.

And just as a reminder, I am desiring the critique to be more from a software engineering principles perspective. IOW, I am not focused on novice Excel macro programmers. Or nitpicking about particular VBA syntax and semantics (unless that specifically relates to DRY, DbC, etc.). The intention is to assist future Java/Scala/Python/etc. software engineers who must create and support VBA code bases. Like me.

Feedback is appreciated.

SIDENOTE: In this submission, to keep the discussion clean, I don't plan to discuss my unique VBA code formatting approach. If you are interested in a discussion around that, let me know in the comments below and I will start a separate review submission for that.


The main function, f_ua_lngSize, just focuses on obtaining the size. The function can be called on any of the Array's dimensions, and defaults to the first.

Public Const M_UA_SIZE_NOT_ARRAY As Long = -1
Public Const M_UA_SIZE_EMPTY As Long = 0

'Return Value:
'   -1 - Not an Array
'    0 - Empty
'  > 0 - Defined
Public Function f_ua_lngSize( _
    ByRef pr_avarValues As Variant _
  , Optional ByVal pv_lngDimensionOneBased As Long = 1 _
) As Long
  Dim lngSize As Long: lngSize = M_UA_SIZE_NOT_ARRAY 'Default to not an Array
  Dim lngLBound As Long
  Dim lngUBound As Long
  
  On Error GoTo Recovery
  
  If (IsArray(pr_avarValues) = True) Then
    lngSize = M_UA_SIZE_EMPTY 'Move default to Empty
    lngLBound = LBound(pr_avarValues, pv_lngDimensionOneBased)
    lngUBound = UBound(pr_avarValues, pv_lngDimensionOneBased)
    If (lngLBound <= lngUBound) Then
      lngSize = lngUBound - lngLBound + 1 'Non-Empty, so return size
    End If
  End If
  
NormalExit:
  f_ua_lngSize = lngSize
  Exit Function
  
Recovery:
  GoTo NormalExit
End Function

Then I've created two helper functions, f_ua_blnIsEmpty and f_ua_blnIsDefined, which just interpret calls to f_ua_lngSize above.

Public Function f_ua_blnIsEmpty( _
    ByRef pr_avarValues As Variant _
  , Optional ByVal pv_lngDimensionOneBased As Long = 1 _
) As Boolean
  f_ua_blnIsEmpty = f_ua_lngSize(pr_avarValues, pv_lngDimensionOneBased) = 0
End Function

Public Function f_ua_blnIsDefined( _
    ByRef pr_avarValues As Variant _
  , Optional ByVal pv_lngDimensionOneBased As Long = 1 _
) As Boolean
  f_ua_blnIsDefined = f_ua_lngSize(pr_avarValues, pv_lngDimensionOneBased) > 0
End Function
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Fwiw, the late Chip Pearson's Functions for VBA arrays cover edge cases very rigorously, although I cannot vouch for how performant they are (that said, I would only focus on performance if this is demonstrably a bottleneck). There are some things I would change and rubberduck would suggest many but it's a good start. \$\endgroup\$
    – Greedo
    Jul 23 at 7:17
  • \$\begingroup\$ @greedo I did have a look at CP's stuff (although I didn't realize he had passed...my condolences). CP's stuff seemed to depend very deeply on Variant which I understood to have very poor performance. I do realize the code above is based on Variant as that is the only way to create collection "generic" code in VBA, from what I can tell. \$\endgroup\$ Jul 23 at 22:11
1
\$\begingroup\$

Once you know you have an array it looks to me that this comparison is unecessary:

If (lngLBound <= lngUBound) Then
  lngSize = lngUBound - lngLBound + 1 'Non-Empty, so return size
End If

The reason being that if you have an uninitialized array then

lngLBound = 0
lngUBound = -1

So your equation evaluates to:

lngSize = -1 - 0 + 1

Which means that lngSize = 0

So it's safe to simplify your main function like so:

Public Function f_ua_lngSize( _
    ByRef pr_avarValues As Variant _
  , Optional ByVal pv_lngDimensionOneBased As Long = 1 _
) As Long
  Dim lngSize As Long: lngSize = M_UA_SIZE_NOT_ARRAY 'Default to not an Array
  Dim lngLBound As Long
  Dim lngUBound As Long
  
  On Error GoTo Recovery
  
  If (IsArray(pr_avarValues) = True) Then
    lngLBound = LBound(pr_avarValues, pv_lngDimensionOneBased)
    lngUBound = UBound(pr_avarValues, pv_lngDimensionOneBased)
    lngSize = lngUBound - lngLBound + 1 ' return size
  End If
  
NormalExit:
  f_ua_lngSize = lngSize
  Exit Function
  
Recovery:
  GoTo NormalExit
End Function

This makes M_UA_SIZE_EMPTY unused.

To prove this out I recommend writing a unit test suite that tests all cases you expect to handle. You can have a test for at least your three states listed above the function.

You can use RubberDuckVBA to help you write and run unit tests. It will also give you some of the modern features you are missing in VBA. WARNING: It will tell you about all the style mistakes you are making that you said you don't want to hear about. So be prepared to be amazed and also prepare to have your feelings hurt.

NOTE: I have answered for performant by reducing an instruction. I have answered safe by suggesting that unit testing is your provable measure of safety. As for robustness I think it does what it says on the tin without side effect so it's good.

\$\endgroup\$
6
  • \$\begingroup\$ I found and have been using RubberDuckVBA for the last month. It definitely enhanced my IDE experience. It didn't move the needle much on VBA itself, not that it really could have. I am curious about the Unit testing, though. I WAY prefer TDD. I use JUnit extensively in Java, and a similar tool in Scala. Is there a link somewhere showing how to get something similar via RubberDuckVBA? A quick look around didn't show anything obvious. \$\endgroup\$ Jul 23 at 21:59
  • \$\begingroup\$ BTW, I went here: rubberduckvba.com/Features/Details/UnitTesting But the branch of links are all 404s. So, any guidance or examples of how to do Unit testing with RubberDuckVBA would be greatly appreciated. \$\endgroup\$ Jul 23 at 22:06
  • 1
    \$\begingroup\$ @chaotic3quilibrium yes, he made a new website and it's been under construction for quite some time. There is tons of good information about unit tests and all things VBA on the authors blog: rubberduckvba.wordpress.com/2017/10/19/… \$\endgroup\$
    – HackSlash
    Jul 23 at 22:07
  • 1
    \$\begingroup\$ You may enjoy his OOP articles. \$\endgroup\$
    – HackSlash
    Jul 23 at 22:11
  • 1
    \$\begingroup\$ @chaotic3quilibrium just FYI, he can hear you. He's quite active here. \$\endgroup\$
    – HackSlash
    Jul 26 at 15:35
0
\$\begingroup\$

@Hackslash The assertion of

lngLBound = 0
lngUBound = -1

for an uninitialised array is incorrect. The code below demonstrates why

Public Sub ArrayTypeInfo()

    Dim myUbound As Long
    Dim myUboundMsg As String
    On Error Resume Next
    Debug.Print , , , "TypeName", "VarType", "IsArray", "IsNull", "IsEmpty", "Ubound"
    
    Dim myLongs() As Long
    myUbound = UBound(myLongs)
    myUboundMsg = IIf(Err.Number = 0, CStr(myUbound), "Ubound error")
    Debug.Print "Dim myLongs() As Long", , TypeName(myLongs), VarType(myLongs), IsArray(myLongs), IsNull(myLongs), IsEmpty(myLongs), myUboundMsg
    On Error GoTo 0
    On Error Resume Next
    
'@Demonstrates nicely the dangers of assumption after on error resume next
'@ its not a Ubound error.  You can't assign an array created using array to an array not declared as variant holding array of x.
    myLongs = Array(1&, 2&, 3&, 4&, 5&)
    myUbound = UBound(myLongs)
    myUboundMsg = IIf(Err.Number = 0, CStr(myUbound), "Ubound error")
    Debug.Print "myLongs = Array(1&, 2&, 3&, 4&, 5&)", TypeName(myLongs), VarType(myLongs), IsArray(myLongs), IsNull(myLongs), IsEmpty(myLongs), myUboundMsg
    On Error GoTo 0
    On Error Resume Next
    
    ReDim myLongs(1 To 5)
    myUbound = UBound(myLongs)
    myUboundMsg = IIf(Err.Number = 0, CStr(myUbound), "Ubound error")
    Debug.Print "ReDim myLongs(1 To 5)", , TypeName(myLongs), VarType(myLongs), IsArray(myLongs), IsNull(myLongs), IsEmpty(myLongs), myUboundMsg
    On Error GoTo 0
    On Error Resume Next
    
    Debug.Print
    Dim myArray As Variant
    myUbound = UBound(myArray)
    myUboundMsg = IIf(Err.Number = 0, CStr(myUbound), "Ubound error")
    Debug.Print "Dim myArray As Variant", , TypeName(myArray), VarType(myArray), IsArray(myArray), IsNull(myArray), IsEmpty(myArray), myUboundMsg
    On Error GoTo 0
    On Error Resume Next
    
    myArray = Array()
    myUbound = UBound(myArray)
    myUboundMsg = IIf(Err.Number = 0, CStr(myUbound), "Ubound error")
    Debug.Print "myArray = Array()", , TypeName(myArray), VarType(myArray), IsArray(myArray), IsNull(myArray), IsEmpty(myArray), myUboundMsg
    On Error GoTo 0
    On Error Resume Next
    
    myArray = Array(1, 2, 3, 4, 5)
    myUbound = UBound(myArray)
    myUboundMsg = IIf(Err.Number = 0, CStr(myUbound), "Ubound error")
    Debug.Print "myArray = Array(1, 2, 3, 4, 5)", TypeName(myArray), VarType(myArray), IsArray(myArray), IsNull(myArray), IsEmpty(myArray), myUboundMsg
    On Error GoTo 0
    On Error Resume Next
    
    myArray = myLongs
    myUbound = UBound(myArray)
    myUboundMsg = IIf(Err.Number = 0, CStr(myUbound), "Ubound error")
    Debug.Print "myArray = myLongs", , TypeName(myArray), VarType(myArray), IsArray(myArray), IsNull(myArray), IsEmpty(myArray), myUboundMsg
    On Error GoTo 0
    On Error Resume Next
    
    Debug.Print
    Dim myArrayOfVar() As Variant
    myUbound = UBound(myArrayOfVar)
    myUboundMsg = IIf(Err.Number = 0, CStr(myUbound), "Ubound error")
    Debug.Print "Dim myArrayOfVar() As Variant", TypeName(myArrayOfVar), VarType(myArrayOfVar), IsArray(myArrayOfVar), IsNull(myArrayOfVar), IsEmpty(myArrayOfVar), myUboundMsg
    On Error GoTo 0
    On Error Resume Next
    
    myArrayOfVar = Array()
    myUbound = UBound(myArrayOfVar)
    myUboundMsg = IIf(Err.Number = 0, CStr(myUbound), "Ubound error")
    Debug.Print "myArrayOfVar = Array()", , TypeName(myArrayOfVar), VarType(myArrayOfVar), IsArray(myArrayOfVar), IsNull(myArrayOfVar), IsEmpty(myArrayOfVar), myUboundMsg
    On Error GoTo 0
    On Error Resume Next
    
    myArrayOfVar = Array(1&, 2&, 3&, 4&, 5&)
    myUbound = UBound(myArrayOfVar)
    myUboundMsg = IIf(Err.Number = 0, CStr(myUbound), "Ubound error")
    Debug.Print "myArrayOfVar = Array(1&, 2&, 3&, 4&, 5&)", TypeName(myArrayOfVar), VarType(myArrayOfVar), IsArray(myArrayOfVar), IsNull(myArrayOfVar), IsEmpty(myArrayOfVar), myUboundMsg
    On Error GoTo 0
    On Error Resume Next
    
End Sub

which gives the output

                                          TypeName      VarType       IsArray       IsNull        IsEmpty       Ubound
Dim myLongs() As Long                     Long()         8195         True          False         False         Ubound error
myLongs = Array(1&, 2&, 3&, 4&, 5&)       Long()         8195         True          False         False         Ubound error
ReDim myLongs(1 To 5)                     Long()         8195         True          False         False         5

Dim myArray As Variant                    Empty          0            False         False         True          Ubound error
myArray = Array()                         Variant()      8204         True          False         False         -1
myArray = Array(1, 2, 3, 4, 5)            Variant()      8204         True          False         False         4
myArray = myLongs                         Long()         8195         True          False         False         5

Dim myArrayOfVar() As Variant             Variant()      8204         True          False         False         Ubound error
myArrayOfVar = Array()                    Variant()      8204         True          False         False         -1
myArrayOfVar = Array(1&, 2&, 3&, 4&, 5&)  Variant()      8204         True          False         False         4

It should be noted that if Ubound gives a result of -1 it is also necessary to check Lbound for a valid result as the following definition is perfectly acceptable in VBA

Dim myArray(-10 to -1,-10 to -1, -10 to -1)

Update 24 July2021 - I made a mistake below and forgot about the 0th index so my assertion that when crossing from negative to positive you only need Ubound-Lbound is wrong, Its still Ubound-Lbound+1 and finally you also need to be aware that if Lbound and Ubound have the same sign then the size can be calculated using Ubound-Lbound+1 BUT if they have opposite signs then size is just Ubound-Lbound.

\$\endgroup\$
7
  • \$\begingroup\$ Wow! Tysvm for your beyond thorough answer. I really appreciate it. I will make a code adjustment to handle the "crossing zero" case you identified. This is an example of an edge case I DID NOT SEE ADDRESSED ANYWHERE ELSE. And that is after looking at hundreds of StackOverflow questions/answers (of which there seems to be a huge number just around VBA Array Size issues - it's like it is the ultimate opportunity to ensure off-by-one issues). \$\endgroup\$ Jul 23 at 22:02
  • 1
    \$\begingroup\$ TIL. I've never seen a negative array index in all my years. I always thought indexes began at 0 (Unsigned int) and everything else was just hand waving. \$\endgroup\$
    – HackSlash
    Jul 23 at 22:11
  • 1
    \$\begingroup\$ Yeah am I being dumb, or does a 1D array with Lbound -1, Ubound 1 not have 3 elements, or 1 - (-1) +1. I.e. the original formula is fine even when crossing zero \$\endgroup\$
    – Greedo
    Jul 24 at 17:00
  • 2
    \$\begingroup\$ Slaps face, puts on pointy hat and goes to stand in corner. I've updated my post to make sure the error is highlighted. \$\endgroup\$
    – Freeflow
    Jul 24 at 17:22
  • 1
    \$\begingroup\$ As a result of analyzing all of this, I did find a small performance improvement. I will post and answer showing the one improvement. \$\endgroup\$ Jul 24 at 17:45
0
\$\begingroup\$

NOTE: Per the CodeReview rules, I am not allowed to modify my original question with suggestions and improvements. And instead, I'm required to post an answer showing any substantial adjustments and/or enhancements. Hence, the post below.


UPDATE 2021/Jul/31

Per deeply appreciated additional feedback from @Freeflow, I have updated the code to simplify the naming scheme (ex: removed the Hungarian Notation).


Thanks to both comments and feedback by @HackSlash and @FreeFlow, it appears the original overall implementation is correct.

However, in my continuous review, I discovered two things that can be changed to reduce the number of instructions executed leading to probably improved performance. A visual diff between the OP (above) and the new code (below) is here.

Public Const SIZE_NOT_ARRAY As Long = -1
Public Const SIZE_EMPTY As Long = 0

'Return Value:
'   -1 - Not an Array
'    0 - Empty
'  > 0 - Defined
Public Function size( _
    ByVal value As Variant _
  , Optional ByVal dimensionOneBased As Long = 1 _
) As Long
  Dim result As Long: result = SIZE_NOT_ARRAY 'Default to not an Array

  Dim lowerBound As Long
  Dim upperBound As Long
  
  On Error GoTo NormalExit
  
  If (IsArray(value) = True) Then
    result = SIZE_EMPTY 'Move default to Empty
    lowerBound = LBound(value, dimensionOneBased) 'Possibly generates error
    upperBound = UBound(value, dimensionOneBased) 'Possibly generates error
    If (lowerBound < upperBound) Then
      result = upperBound - lowerBound + 1 'Size greater than 1
    Else
      If (lowerBound = upperBound) Then
        result = 1 'Size equal to 1
      End If
    End If
  End If
  
NormalExit:
  size = result
End Function

Public Function isEmpty( _
    ByVal value As Variant _
  , Optional ByVal dimensionOneBased As Long = 1 _
) As Boolean
  isEmpty = size(value, dimensionOneBased) = 0
End Function

Public Function isDefined( _
    ByVal value As Variant _
  , Optional ByVal dimensionOneBased As Long = 1 _
) As Boolean
  isDefined = size(value, dimensionOneBased) > 0
End Function
\$\endgroup\$
6
  • 1
    \$\begingroup\$ COngratulations. You should still be aware that your code is still very hard to read as it is laced with 'mysterios' prefixes and other hungarion type notation. For comparison take a look as my TryGetSize Function ``` \$\endgroup\$
    – Freeflow
    Jul 25 at 20:53
  • \$\begingroup\$ @FreeFlow Yes. I am experimenting with a strongly typed VBA which makes Java/Scala programmers more at home, given how poorly the VBA IDE surfaces information about the types. Rubberduck helps, but doesn't currently enable easily seeing the context and types of things directly in the code. IOW, I cannot hover over a variable and see critical aspects to understand with to use it. And given it isn't an object, I don't get intellisense assistance to be able to narrow context to what "methods" would be useful to use on the variable/function-result/etc. \$\endgroup\$ Jul 26 at 19:25
  • \$\begingroup\$ @FreeFlow And I didn't understand your function at all. While the variables were readable, they referred to "hidden" things for which I have ZERO context and are unable to clarify without seeing the rest of your codebase AND jumping into your definitions. IOW, while you might be elevating local readability slighty better by variable/function/etc. name, my clarifying the meaning makes me jump to other places in the code to get the clarifying context. As a Java/Scala coder with decades of experience, I want/need the context at the use site. Hence, my ongoing experimentation. \$\endgroup\$ Jul 26 at 19:28
  • 1
    \$\begingroup\$ LMAO! I just returned to Rubberduck and found that it does in fact show the return type for functions (right next to the "Ready" refresh button). That is SUPER helpful. There is still a ton of context not supplied that I want at the use sites. Like is a function referentially transparent, or not? Does it or one of the things it calls modify the spreadsheet, versus it just does something without modifying the spreadsheet state. These are really important things to avoid breaking my flow as I am coding. \$\endgroup\$ Jul 26 at 19:35
  • \$\begingroup\$ Given the rant you had last time, not a chance. \$\endgroup\$
    – Freeflow
    Jul 31 at 15:53
-1
\$\begingroup\$

Your code is still laced with very ugly, impenetrable prefixes and other 'hungation nototation' stuff.. Try comparing the readability of your code with my TryGetSize function

'@Description("Returns the count of items in an iterable")
Public Function TryGetSize(ByVal ipIterable As Variant, Optional ByRef iopResult As ResultLong, Optional ByVal ipRank As Long = 1) As ResultLong

    If iopResult Is Nothing Then Set iopResult = ResultLong.Deb.SetFailure
    Set TryGetSize = iopResult
    
    If Types.Group.IsNotIterable(ipIterable) Then Exit Function
    
    If Types.IsArray(ipIterable) Then
    
        Arrays.TryGetSize ipIterable, ipRank, iopResult
 
    Else
    
        iopResult.SetSuccess ipIterable.Count
        
    End If
     
End Function

Of course for the above code there is a fair bit of backing library code.

\$\endgroup\$
9
  • \$\begingroup\$ I didn't understand your function at all. While the variables were readable, they referred to "hidden" things for which I have ZERO context and are unable to clarify without seeing the rest of your codebase AND jumping into your definitions. More details in comments here: codereview.stackexchange.com/a/264359/4758 \$\endgroup\$ Jul 26 at 20:18
  • 1
    \$\begingroup\$ @chaotic3quilibrium I'm sorry you are feeling frustrated by the VBA Ide. For the code I presented everything is discoverable through Intellisense, and the detail through F2 and Shift F2. I'm sorry now I didn't make this clearer. I've never had the advantage of more sophisticated IDE's as I'm just a hobbyist programmer so have learnt how to make the best use of the limited facilities that the VBA ide does present. I did acknowledge that the code I provided was supported by a fair bit of backing Library code. \$\endgroup\$
    – Freeflow
    Jul 27 at 20:14
  • \$\begingroup\$ Per your comment about my naming convention experiments and now that I have far more Rubberduck IDE experience, I have now refactored the OP function. Please have a look at this Gist and let me know if you think it is more readable. And also if you have any other feedback about it. gist.github.com/jim-oflaherty-jr-qalocate-com/… \$\endgroup\$ Jul 31 at 14:49
  • \$\begingroup\$ @Freeflow: I'm interested in your Types.Group. idea (and maybe more around). Could you somewhere/somehow explain a bit about 'what and how' is in your lib? \$\endgroup\$ Aug 7 at 11:44
  • \$\begingroup\$ @UnhandledException No problem. I'll open a code review item tomorrow and expose the ghastly details to view. I'll post a link here when I've done so. \$\endgroup\$
    – Freeflow
    Aug 8 at 16:25

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.