-
Notifications
You must be signed in to change notification settings - Fork 1
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
Comments
As for |
Following #5 (comment), we can name the arguments |
What about if neither Should |
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 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 |
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}); |
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 If we completely disregard this point, I speculate that most people would prefer to have the |
Review:
Other consideration: |
Using On the other hand, introducing To me, both approaches break conventions in their own way. However, I personally lean towards Additionally, instance methods are easily expandable to |
@hax I have revised it into a table It seems that the most unconventional ones are the most useful |
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. 🤪 |
I just added number. |
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. |
I'm going to retract this statement I said earlier:
I've gotten pretty used to reading and writing code that uses |
It's an interesting idea (and would obviate #11) to have |
I claim that CSS parameter order (lower bound, number to clamp, upper bound) is the only generally intuitive order. What I mean by that is the order that someone would guess the parameters to be in with no other context or programming experience. To my knowledge, across cultures, even those using right-to-left writing systems, real numbers are depicted on a number line going from low numbers on the left to high numbers on the right. Using the number line as intuition, it makes sense that you would pass clamp a low bound on the left and a high bound on the right, with the number that is supposed to be between them... between them. Failing agreement on that, I think we'd have to end up going with the prototype method, which again, obviously would take the lower bound on the left and the upper bound on the right. |
@michaelficarra I noticed that despite your "intuitive" order being a part of the spec for some time, every single commenter used (num, min, max) order anyways. Not a single person noticed that the order was actually (min, num, max), even though it was. So, I changed it to the one they were using. |
The number-line example does fall apart when you consider the fact that the value you're trying to clamp isn't actually clamped yet. It could be smaller than the minimum number or larger than the maximum number. It'll only be between the two numbers after the clamping operation has finished. There are other intuitive reasons one would expect the value being clamped to come first. And I think it's fair to use a programmer's intution here since we are programmers. When we write static methods, we typically place the subject of the method as the first argument. Some examples:
There's a really strong presedence for doing this sort of thing. Clamping isn't all that different in this regard - the value we're trying to clamp would come first, and then other parameters explaining how the clamping operation should be performed would come afterwards. That being said, I see what you're saying and understand why you feel it is intuitive. But there's other "intuitive" ways to look at this issue. |
The natural-language interpretation of this function is "clamp |
Updated comparison, after ruling out option bags and multiple functions from #4 (comment)
|
I believe the argument for |
One of the people who originally added the CSS function has said that with hindsight the weight of precedent suggested other order, CSS functions aren't much like JS functions anyway, and we should just do the obvious thing in JS i.e. We should not be willing to break precedent with every other language just to match CSS here. |
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 })
orvalue.clamp(min, max)
. These alternatives provide a clearer way of expressing the function parameters.The text was updated successfully, but these errors were encountered: