Thanks for the feedback guys.

Based on the discussion I propose a different set of changes:

1.
shuffle
^ self shuffleBy: Random new

2.
shuffled
^ self copy shuffle

3. remove #shuffledBy: (if you're specific enough to use a custom Random instance you can also create a copy of the collection yourself if you want to)


I leave the matter of renaming the methods for further discussion. Personally I'm for more intention revealing selectors; I quite like #shuffle and #shuffleInPlace. If we can agree on a different naming scheme we should then apply it to other methods aswell of course (e.g. #sort, #sortInPlace).

Also open for discussion is the use of Collection>>randomForPicking and Collection>>mutexForPicking in other methods (such as #atRandom). I think it shouldn't be too big a problem to make those methods use individual Random instances and to remove the two class variables from Collection.

Cheers,
Max


On 24.09.2013, at 02:33, btc@openInWorld.com wrote:

Nicolas Cellier wrote: 
The reason why I don't like the explicit "copy", is because this is the
default behaviour.
In place mutation is an (evil) exception.
I'm personnally fine with current conventions:
- sort, shuffle, reverse, replace: are imperative, thus you are telling
collection, sort yourself !
- sorted clearly sounds as a sorted copy for me.
If ever a change was to be made, I would much prefer this to happen on
opposite naming, sort -> inPlaceSort, or sortYourself :)
It's like telling the programmer, hey - a stateful mutation is really what
you want, is it?

Conventions should encourage good programming practice.  Although it makes perfect sense on hearing it, I was explicitly not aware of the convention between 'sort' and 'sorted'.  For me, the small difference in letters used between the two doesn't provide a wide enough distinction.  Also, regarding not mutating state, I guess I knew that in the back of my head, but it has not generally been an explicit concern of mine while programming.  Convention should encourage good programming.  So I like the suggestion of inPlaceSort & sortYourself.  As well as being more intention revealing, the less desirable methods are made less attractive by using a longer name. Perhaps another naming convention could be mutatedSort, or mutSort as a hint that the programmer should consider using these in conjunction with mutexes, or at own risk.

cheers -ben



2013/9/23 St�phane Ducasse <stephane.ducasse@inria.fr>

  
+1
Now I understand the point of max with the name sort/sorted because this
is not explicit to me too :)
I laways have to think.

On Sep 23, 2013, at 6:28 PM, Nicolas Cellier <
nicolas.cellier.aka.nice@gmail.com> wrote:

100% agree.
Do it right > do it fast.
We must not turn usage of the library into something fragile for the sake
of speed.
We already make the code itself fragile more often than not in term of
complexity (harder to understand/test/change).

Especially, introduction of shared mutable states (global, class vars,
singleton or any other form) should ring an alarm in reviewers head
(This is some very old Squeak code in this case, so Squeakers are to
blame, but we're all in same bath).



2013/9/23 Henrik Johansen <henrik.s.johansen@veloxit.no>

    
On Sep 23, 2013, at 3:33 , Max Leske <maxleske@gmail.com> wrote:

      
If a user is going to modify the same object concurrently, he/she
          
takes care of mutual exclusion.
      
Agreed. BUT: the Random object used by these methods is the same one
        
that is used by #atRandom for instance, hence the race condition. There is
no way anyone can safely use these methods without the mutex, single
threaded or not. Calls to methods using that same Random object can be all
over the place and also in the base system.


It seems to me an existing Random instance is used in this case mostly*
for performance.
One could argue that since the Random in this case is used for a bulk
operation, for which the object creation cost is largely amortized for
collection sizes > 20, it's acceptable to instead use Random new by
default, which wouldn't suffer from the same race conditions.
While still slower than a mutex-protected version for single-threaded
code, it would also scale correctly if the users (and vm) are actually
multi-threaded.

[#(1 2  3 4 5 6 7 8 9 10 11 12) shuffle] bench '208,000 per second.'
 '222,000 per second.' '223,000 per second.'
[#(1 2 3 4 5 6 7 8 9 10 11 12) shuffleWithMutex] bench '188,000 per
second.'  '186,000 per second.''184,000 per second.'
[#(1 2 3 4 5 6 7 8 9 10 11 12) shuffleNewRandom] bench '167,000 per
second.' '166,000 per second.'  '167,000 per second.'

Cheers,
Henry

* Low seed entropy is another issue, but if purely random shuffling is a
critical requirement, one shouldn't use the default Random generator
anyways...