sancarn / stdVBA

VBA Standard Library - A Collection of libraries to form a common standard layer for modern VBA applications.
MIT License
288 stars 60 forks source link

Bug/Unexpected Behaviour when using stdArray.Reduce #58

Closed ThomasG08 closed 2 years ago

ThomasG08 commented 2 years ago

The stdArray.Reduce method exhibits the following unexpected behaviour because the intitialValue Parameter is always predeclared as zero:

  1. minimum value of an array of positive integers always returns zero, even if it was not included in the original array
  2. maximum value of an array of negative integers always returns zero, even if not in the original array.

The following calls would both print a zero, while the expected answer would be 1 and -1 respectively: Debug.Print stdArray.Create(1, 2, 3, 4, 5).reduce(stdLambda.Create("If $1 < $2 Then $1 else $2")) Debug.Print stdArray.Create(-1, -2, -3, -4, -5).reduce(stdLambda.Create("If $1 > $2 Then $1 else $2"))

In order to get the correct answer one would have to set initialValue as follows: to a value >= the minimum to calculate the minimum to a value <= the maximum to calculate the maximum but both of these require that the answer is already known.

Standard behaviour of JS/Java would be to have initialValue as an optional parameter and start with the first element of the array if initialValue is not passed in.

https://github.com/sancarn/stdVBA/blob/f7d4594be429e9341d5469bd34d042741f8ff320/src/stdArray.cls#L704

sancarn commented 2 years ago

Standard behaviour of JS/Java would be to have initialValue as an optional parameter and start with the first element of the array if initialValue is not passed in.

Oh really? Well that is likely a breaking change... Not sure how significantly it would impacy, but more than happy for the algorithm to be reworked to fix the issue.

Need consideration as to how this is applied with stdEnumerator

ThomasG08 commented 2 years ago

I found some documentation on the matter here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/Reduce

initialValue Optional A value to which previousValue is initialized the first time the callback is called. If initialValue is specified, that also causes currentValue to be initialized to the first value in the array. If initialValue is not specified, previousValue is initialized to the first value in the array, and currentValue is initialized to the second value in the array.

I did some experiments in the testBuilder.xlsm you provided and came up with this changed version. I ran the tests in the stdArrayTests Module and they all passed, although you don't have a lot of tests for the reduce method specifically.

Public Function reduce(ByVal cb As stdICallable, Optional ByVal initialValue As Variant) As Variant  ' = 0
  If pInitialised Then
    'reduce = initialValue  'REMOVED

    Dim i As Integer
    For i = 1 To pLength
      'BUGFIX: Sometimes required, not sure when
      Dim el As Variant
      CopyVariant el, pArr(i)

      If i = 1 Then                                                                                   'ADDED
        If IsMissing(initialValue) Then CopyVariant reduce, el Else CopyVariant reduce, initialValue  'ADDED
      End If                                                                                          'ADDED
      If Not (i = 1 And IsMissing(initialValue)) Then CopyVariant reduce, cb.Run(reduce, el)          'ADDED

      'Reduce                         'REMOVED
      'reduce = cb.Run(reduce, el)    'REMOVED
    Next
  Else
    'Error
  End If

End Function

It should also enhance the feature as the change would allow the accumulator to be an object, not just a primitive type.

sancarn commented 2 years ago

Hmm... I'd prefer not to check the condition every loop... Perhaps:


Public Function Reduce(ByVal cb As stdICallable, Optional ByVal initialValue As Variant=0) As Variant
  Dim iStart as long
  If pInitialised Then
    if pLength > 0 then
      if isMissing(initialValue) then 
        Call CopyVariant(Reduce, pArr(1))
        iStart = 2
      else
        Call CopyVariant(Reduce, initialValue)
        iStart = 1
      end if
    else
      if isMissing(initialValue) then
        Reduce = Empty
      else
        Call CopyVariant(Reduce, initialValue)
      end if
      Exit Function
    end if 

    Dim i As long
    For i = iStart To pLength
      'BUGFIX: Sometimes required, not sure when
      Dim el As Variant
      CopyVariant el, pArr(i)

      'Reduce
      Reduce = cb.Run(Reduce, el)
    Next
  Else
    'Error
  End If
End Function
sancarn commented 2 years ago

I've got a fixed branch on stdArray-Fix#58 - currently awaiting testing before commit 😊 see #60

sancarn commented 2 years ago

So unfortunately because of #55 the use of objects in the reduce function is highly limited still, mainly because we still don't have a means of triggering the let/set keywords. I.E. stuff like this would be impossible:

[{}, {k: "A", v: 1}, {k: "B", v: "hello"}].reduce((a,b)=>{a[b.k] = b.v; return a})

That said, I'll be uploading this branch soon and closing out this issue 😊 Thanks for the report.