Bug/Unexpected Behaviour when using stdArray.Reduce
ThomasG08 opened this issue · 5 comments
The stdArray.Reduce method exhibits the following unexpected behaviour because the intitialValue Parameter is always predeclared as zero:
- minimum value of an array of positive integers always returns zero, even if it was not included in the original array
- 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.
Line 704 in f7d4594
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
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.
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
I've got a fixed branch on stdArray-Fix#58 - currently awaiting testing before commit 😊 see #60
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.