another confusing result

This may just be me misunderstanding the way it's supposed to work, but I was exploring the method 

On Jan 8, 2012, at 4:16 PM, Larry White wrote:

And here, if you do:
'abcde' combinations: 2 atATimeDo: [:each | each inspect. Transcript cr; show: each printString].
You get inspectors with #($e $e) ...

>
> but, if you add :each to a collection instead of printing, you get something different. The following code:
> res := OrderedCollection new.
>  'abcde' combinations: 2 atATimeDo: [:each | res add: each].
> Transcript show: res.
> produces:
> an OrderedCollection(#($e $e) #($e $e) #($e $e) #($e $e) #($e $e) #($e $e) #($e $e) #($e $e) #($e $e) #($e $e))
>
> Thanks again.


After a quick look at the methods

combinations: kk atATimeDo: aBlock
        "Take the items in the receiver, kk at a time, and evaluate the block for each combination.  Hand in an array of elements of self as the block argument.  Each combination only occurs once, and order of the elements does not matter.  There are (self size take: kk) combinations."
        " 'abcde' combinations: 3 atATimeDo: [:each | Transcript cr; show: each printString]"

        | aCollection |
        aCollection := Array new: kk.
        self combinationsAt: 1 in: aCollection after: 0 do: aBlock

and

combinationsAt: jj in: aCollection after: nn do: aBlock
        "Choose k of N items and put in aCollection.  jj-1 already chosen.  Indexes of items are in numerical order, to avoid the same combo being used twice.  In this slot, we are allowed to use items in self indexed by nn+1 to self size.  nn is the index used for position jj-1."
        "(1 to: 6) combinationsSize: 3 do: [:each | Transcript cr; show: each printString]"

nn+1 to: self size do: [:index |
                aCollection at: jj put: (self at: index).
                jj = aCollection size
                        ifTrue: [aBlock value: aCollection]
                        ifFalse: [self combinationsAt: jj + 1 in: aCollection after: index do: aBlock]].

You can see that each step result is store in the same collection.
So actually, when you print it it's ok, because it's done before the following step erase the current content of the collection.
But then at the end, you get always the same result, it's what the code is supposed to do.

Then, I do not think it makes sense, but at least, now I understand why ;)


Ben

Here is a copy of something I wrote about this earlier:

In Pharo 1.3, SequenceableCollection>>combinations:atATimeDo: will,
for all combinations, send the same collection instance as argument
for the block. Therefore, if you use this collection itself, you will
be surprised:

|answer|
answer := OrderedCollection new.
#(a b c) combinations: 2 atATimeDo: [:each | answer add: each].
answer

Here I expect to end up with a collection looking like this:
1:  #(#a #b)
2:  #(#a #c)
3:  #(#b #c)

But I end up with:
1:  #(#c #c)
2:  #(#c #c)
3:  #(#c #c)

One fix is to change the client code to copy the argument for the block:
|answer|
answer := OrderedCollection new.
#(a b c) combinations: 2 atATimeDo: [:each | answer add: each copy].
answer

Another solution is enhancing
SequenceableCollection>>combinationsAt:in:after:do: to copy the
collection:
nn + 1 to: self size do: [ :index |
               aCollection at: jj put: (self at: index).
               jj = aCollection size
                       ifTrue: [ aBlock value: aCollection copy ]
                       ifFalse: [
                               self
                                       combinationsAt: jj + 1
                                       in: aCollection
                                       after: index
                                       do: aBlock ] ]



Kind regards
Runar Jordahl

Thanks.  I wonder if this method needs to stay the same for compatibility reasons or if it could be modified in Pharo to produce a less surprising result. In other words, is this a bug or a feature?