Replace #min:max with #clampLow:High:

[bencoman]

Moved from https://pharo.fogbugz.com/f/cases/21758/Replace-min-max-with-clampLow-High

On Sat, 21 Apr 2018, Ben Coman wrote:

On 21 April 2018 at 03:51, Hilaire hilaire@drgeo.eu wrote:
Hi,

 Out of curiosity.
 I always found the #min:max: confusing and lost in its expressiveness.
 One should write:

     10 min: 48 max: 12

 to expect 12.
 but logically one (at least me) may want to express it as:

     10 min: 12 max: 48

 Then when reading its source code, it is even more confusing:

 min: aMin max: aMax
     ^ (self min: aMin) max: aMax

 Are not the argument names inversed in their meaning, if any?

I would agree. I see most use by Color like...

Color>>adjustBrightness: brightness
"Adjust the relative brightness of this color. (lowest value is 0.005 so that hue information is not lost)"

   ^ self class
           h: self hue
           s: self saturation
           v: (self brightness + brightness min: 1.0 max: 0.005)
           alpha: self alpha

Trying to read that twists my brain.

I can understand the intent from the implementation
min: aMin max: aMax
^ (self min: aMin) max: aMax

but that message might more properly be #min:thenMax:
However something like
(self brightness + brightness boundedBy: 0.005 and: 1.0)
(self brightness + brightness boundedMin: 0.005 max: 1.0)
seems more intention revealing.

Altering #min:max semantics would make awful portability,
but perhaps it should forward to a new method to make it clear the other is preferred.

Would the Squeak community be amenable to a similar change?
I'd be happy to contribute the changes to the Squeak Inbox.

On 21 April 2018 at 17:56, Levente Uzonyi leves@caesar.elte.hu wrote:

Squeak has #clampLow:high: for this reason.

Levente

[bencoman]

Peter says...

Here's another option:

add #clampLow:high: method

change #min:max: to

min: aMin max: aMax
^ self clampLow: aMin max: aMax

that way

• people looking for the method can still find it (I don't think anyone looking for the current behavior would think about voltages, but about mins and maxes (and I suppose the non-discoverability applies to #beBetween:and: too)
• when you look at the implementation, it is immediately obvious what it does

[stale]

To limit bug bankruptcy (see https://www.joelonsoftware.com/2012/07/09/software-inventory/) this issue has been automatically marked as stale because it has not had any activity in 6 months. It will be closed in 1 month if no further activity occurs. If this issue remains important to you, please comment to reactivate the issue. Thank you for your contributions.

Joel on Software

Software Inventory

Imagine, for a moment, that you came upon a bread factory for the first time. At first it just looks like a jumble of incomprehensible machinery with a few people buzzing around. As your eyes adjus…

[hilaire]

Done.
By the way checking out of this branch code crashes the image. An atomic problem with min:max: been used while clampLow:high: method is removed?

[Ducasse]

min:max: is probably used in the kernel deep down somewhere.

[Ducasse]

Hi ben can you have a look at the discussion in the PR because now I understand much better and I think that I have a better proposal.

The solution would be

• min:max: -> renamed min:thenMax:
• add beBetween:and: with a different semantics.