CanadaHonk / proposal-math-clamp

A TC39 proposal to add Math.clamp
https://canadahonk.github.io/proposal-math-clamp/
MIT License
36 stars 1 forks source link

Three arguments of the same type is confusing #4

Open hax opened 8 months ago

hax commented 8 months ago

The function currently includes three arguments of the same type, making it difficult to determine the specific meanings of each parameter when calling the function as Math.clamp(a, b, c). Both the parameter order (value, min, max) and (min, value, max) can confuse some developers because they may expect the opposite.

It might be worth considering alternative approaches such as Math.clamp(value, { min, max }) or value.clamp(min, max). These alternatives provide a clearer way of expressing the function parameters.

Richienb commented 8 months ago

value.clamp might be out of the question because there only exist to* methods on Number.prototype.

As for {min, max}, objects are already used in some navigator functions so it wouldn't be that unjavascripty. It would also allow either min or max to be left unspecified so as to allow for more explicit forms: Math.clamp(value, {min: 0}) === Math.max(0, value).

Richienb commented 8 months ago

Following https://github.com/Richienb/proposal-math-clamp/issues/5#issuecomment-1895781703, we can name the arguments number, min and max.

Richienb commented 8 months ago

What about if neither min nor max is provided by the user? What about if no object at all is provided by the user?

Should Math.clamp return the number untouched, or throw a TypeError?

theScottyJam commented 8 months ago

It is true that all parameters are numeric types, but generally you'd still be able to easily figure out what the order is by reading the code.

For example:

character.pos.x = Math.clamp(character.pos.x, 0, SCREEN_WIDTH);

Or:

return Math.clamp(angle + offset, 0, 360);

I don't think it's too difficult in those examples to figure out which parameter does what, even if you would expect Math.clamp()'s function signature to be a little different.

That being said, I'm not overly tied to this stance - I could see some potential readability benefit to explicitly labeling "min" and "max", even if it's pretty easy to figure out which one does which.

(I also don't find the use-case of omitting "min" or "max" arguments overly compelling either, since we already have a Math.min() and Math.max() function to handle that use-case. Perhaps it might be useful to dynamically omit the min or max side based on some condition, but that's already possible with Infinity, e.g. Math.clamp(value, 0, condition ? 360 : Infinity).)

Richienb commented 8 months ago

(I also don't find the use-case of omitting "min" or "max" arguments overly compelling either, since we already have a Math.min() and Math.max() function to handle that use-case. Perhaps it might be useful to dynamically omit the min or max side based on some condition, but that's already possible with Infinity, e.g. Math.clamp(value, 0, condition ? 360 : Infinity).)

The use case is to decrease cognitive load.

Before:

// Clamp score to a lower bound of 0
Math.max(score, 0);

After:

math.clamp(score, {min: 0});
hax commented 8 months ago

you'd still be able to easily figure out what the order is by reading the code.

While carefully reading the code can help understand the purpose of each parameter, I still believe that having all parameters as variables can be problematic during code reviews, especially when there is a mismatch between the intention and the actual invocation (i.e., how can you tell if a code order is actually incorrect?).

Furthermore, since the motivation stated in the README is to emulate CSS clamp, using a parameter order different from CSS clamp could potentially be confusing.

If we completely disregard this point, I speculate that most people would prefer to have the value parameter in the first position. However, even with that, I always strive to minimize such cognitive overhead.

Richienb commented 8 months ago

Review:

Method Only specify max Unambiguous order Easily extendable to BigInt Conventional
number.clamp(min, max) ðŸŸĄ ✔ïļ ✔ïļ ðŸŸĄ Number prototype
number.clamp({min, max}) ✔ïļ ✔ïļ ✔ïļ ðŸŸĄ Option bag, number prototype
Math.clamp(number, {min, max}) ✔ïļ ✔ïļ ❌ ðŸŸĄ Option bag
Math.clamp(number, min, max) ðŸŸĄ ❌ ❌ ✔ïļ
Math.clamp(min, number, max) ðŸŸĄ ❌ ❌ ✔ïļâœ”ïļ CSS signature
Math.clamp(number, min, max), Math.clampMin(number, min), Math.clampMax(number, max) ✔ïļ ❌ ❌ ðŸŸĄ Number prototype, but string.trim/trimStart/trimEnd
number.clamp(min, max), number.clampMin(min), number.clampMax(max) ✔ïļ ✔ïļ ✔ïļ ðŸŸĄ Number prototype, but string.trim/trimStart/trimEnd

Other consideration: Math.constrain alternate name.

hax commented 8 months ago

Using {min, max} as an option bag does have a drawback in the context of the Math.xxx methods. Although option bags are not rare in the language's standard library, they are not prevalent in the Math.xxx methods specifically.

On the other hand, introducing Number.prototype.clamp also goes against the convention in some sense since all the existing mathematical functions are under the Math object rather than Number.prototype.

To me, both approaches break conventions in their own way. However, I personally lean towards Number.prototype.clamp because value.clamp(min, max) is more concise and simpler to write compared to Math.clamp(value, { min, max }).

Additionally, instance methods are easily expandable to bigInt.clamp(), whereas it would be a question whether Math.clamp would support bigint since none of the Math methods currently do. Although some TC39 representatives have expressed a desire to extend the Math methods to support bigint, I anticipate that this is a challenging and potentially impractical problem. Considering future additions like Decimal, in the long run, it might become unavoidable to move the computational methods to the prototypes.

Richienb commented 8 months ago

@hax I have revised it into a table

It seems that the most unconventional ones are the most useful Are you familiar with how this sort of trade-off is made?

hax commented 8 months ago

@Richienb

To be honest, each TC39 representatives are always different, making it difficult to determine which trade-off will ultimately be accepted. Sometimes, the outcome relies on the determination of the champion and the strength of opposition. Therefore, based on my observations, the final form of a proposal can sometimes be somewhat arbitrary. ðŸĪŠ

Richienb commented 8 months ago

I just added number.clamp/clampMin/clampMax to the table, and it seems like it covers everything...

Richienb commented 6 months ago

Virtually all userland implementations order them like (number, min, max) which makes me think that in practice, order ambiguity might not be as big of a problem if we just order them like that as well.

theScottyJam commented 1 day ago

I'm going to retract this statement I said earlier:

(I also don't find the use-case of omitting "min" or "max" arguments overly compelling either, since we already have a Math.min() and Math.max() function to handle that use-case. Perhaps it might be useful to dynamically omit the min or max side based on some condition, but that's already possible with Infinity, e.g. Math.clamp(value, 0, condition ? 360 : Infinity).)

I've gotten pretty used to reading and writing code that uses Math.min() and Math.max() to handle clamping, and I don't have any practice with a clamp() method yet, so it took me a while to come around to it, but I can see now how it's easier to read understand code that's uses an explicit clamp() function with some arguments omitted as opposed to using min() or max() for clamping. With a clamp function, regardless of how the final function signature looks like, it's pretty easy to read it and picture "this parameter is the lower bound while this one is the upper bound", while with something like Math.max(n, 10) it does take me a moment to figure out if the 10 is acting like a lower or upper bound - I basically have to run an example scenario through my head to remember how it works (I'm taking the max of these two numbers, so if n is, say, 20, then the max of 20 and 10 would be 20, which means the 10 would be operating as a lower bound).

ljharb commented 1 day ago

It's an interesting idea (and would obviate #11) to have Number.prototype.clamp and BigInt.prototype.clamp.