Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Three arguments of the same type is confusing #4

Open
hax opened this issue Jan 17, 2024 · 14 comments
Open

Three arguments of the same type is confusing #4

hax opened this issue Jan 17, 2024 · 14 comments

Comments

@hax
Copy link

hax commented Jan 17, 2024

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 Richienb changed the title three arguments with same type is confusing Three arguments of the same type is confusing Jan 17, 2024
@Richienb
Copy link
Collaborator

Richienb commented Jan 17, 2024

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
Copy link
Collaborator

Following #5 (comment), we can name the arguments number, min and max.

@Richienb
Copy link
Collaborator

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
Copy link

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
Copy link
Collaborator

(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
Copy link
Author

hax commented Jan 18, 2024

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
Copy link
Collaborator

Richienb commented Jan 18, 2024

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
Copy link
Author

hax commented Jan 18, 2024

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
Copy link
Collaborator

@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
Copy link
Author

hax commented Jan 18, 2024

@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
Copy link
Collaborator

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

@Richienb
Copy link
Collaborator

Richienb commented Apr 6, 2024

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
Copy link

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
Copy link

ljharb commented Oct 10, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants