On Thu, 8 Sep 2011, Eliot Miranda wrote:

> On Thu, Sep 8, 2011 at 2:18 PM, Levente Uzonyi <leves at elte.hu> wrote:
>
>> On Thu, 8 Sep 2011, Eliot Miranda wrote:
>>
>>  Levente,  I object :)  I find the findBinary:do:ifNone: extremely ugly and
>>> much prefer the findNearbyBinaryIndex: usage.  Would you mind if we
>>> reverted
>>> to that?
>>>
>>
>> I pushed it to the inbox, so there's nothing to revert.
>
>
> Ah, sorry.  Saw the commit notice and assumed.  Apologies.
>
>
>> I prefer to have as few implementations of binary search as possible.
>>
>
> Yes, but findNearbyBinaryIndex: pays its way.  It saved 11 lines and is way
> more comprehensible.

I didn't even try to save lines (which highly depend on indentation), but 
to use existing methods, simple blocks and the stack (no return from 
blocks).

To keep only one binary search implementation and also keep 
#findNearbyBinaryIndex:, I suggest a new implementation for it:

 	^self
 		findBinaryIndex: aBlock
 		ifNone: [ :lower :upper |
 			lower = self size
 				ifTrue: [ upper ]
 				ifFalse: [ lower ] ]

This is only 6 lines long, but could be just one if you like it that way:

 	^self findBinaryIndex: aBlock ifNone: [ :l :u | l = self size ifTrue: [ u ] ifFalse: [ l ] ]

instead of 12. :)

>
>
>> Did you check the other changes? Are those acceptable?
>>
>
> I'm mulling them over :)  I like a variable for the context pc (which Henrik
> didn't suggest) and I like using an at:ifAbsent: block.  So I'll hopefully
> submit a new version sometime soon.

Thanks. I found that my code has a bug. #detectMax: returns the 
association instead of the maximum value according to the block, so some 
extra sends are necessary that way.


Levente

snip