Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug/Unexpected Behaviour when using stdArray.Reduce #58

Closed
ThomasG08 opened this issue Aug 30, 2022 · 5 comments
Closed

Bug/Unexpected Behaviour when using stdArray.Reduce #58

ThomasG08 opened this issue Aug 30, 2022 · 5 comments
Labels
bug Something isn't working good first issue Good for newcomers lib-stdArray lib-stdEnumerator

Comments

@ThomasG08
Copy link

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.

Public Function Reduce(ByVal cb As stdICallable, Optional ByVal initialValue As Variant=0) As Variant

@sancarn
Copy link
Owner

sancarn commented Aug 30, 2022

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

@sancarn sancarn added bug Something isn't working good first issue Good for newcomers lib-stdArray lib-stdEnumerator labels Aug 30, 2022
@ThomasG08
Copy link
Author

ThomasG08 commented Aug 30, 2022

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
Copy link
Owner

sancarn commented Aug 30, 2022

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
Copy link
Owner

sancarn commented Aug 30, 2022

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

@sancarn
Copy link
Owner

sancarn commented Aug 31, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers lib-stdArray lib-stdEnumerator
Projects
None yet
Development

No branches or pull requests

2 participants