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

[Feature]: common > utils > clamp.ts Amendment request #483

Open
1 of 2 tasks
yu-ratel opened this issue May 28, 2024 · 2 comments
Open
1 of 2 tasks

[Feature]: common > utils > clamp.ts Amendment request #483

yu-ratel opened this issue May 28, 2024 · 2 comments

Comments

@yu-ratel
Copy link

Package Scope

  • Add to an existing package
  • New package

Package name: @toss/utils

Overview

In clamp.ts

Hello!

I'm a student studying the toss/slash function and I came across the clamp function's source code. Looking at the bound1 and bound2 parameters, I find them difficult to predict exactly. I'm suggesting changing them to explicit parameter names for better clarity. Additionally, currently, bound2 is an optional property, so if it's not specified, it returns the minimum value. Therefore, I think it would be better to update the description to say that if the value is greater than the maximum, it returns the maximum value, otherwise, it returns the minimum value. What do you think?

I'm enjoying studying your great code. Thank you!😀😀😀

Describe the solution you'd like

export function clamp(value: number, min: number, max?: number) {
  if (max == null) {
    return Math.min(value, min);
  }

  if (max < min) {
    throw new Error('The value of max must be a number greater than min.');
  }

  return Math.min(Math.max(value, min), max);
}
export function clamp(value: number, lowerBound: number, upperBound?: number) {
  if (upperBound == null) {
    return Math.min(value, lowerBound);
  }

  if (upperBound < lowerBound) {
    throw new Error('The value of upperBound must be a number greater than lowerBound.');
  }

  return Math.min(Math.max(value, lowerBound), upperBound);
}
  • ko.md
    어떤 값의 최댓값, 최솟값을 설정합니다. 그 값이 최댓값보다 크다면, 최댓값을 반환합니다. 그렇지않다면 최솟값을 반환합니다.
  • en.md
    If the value is greater than its upper bound, it returns its upper bound. Otherwise, it returns its lower bound

Additional context

@okinawaa
Copy link
Member

okinawaa commented Jun 1, 2024

Considering the role of the function, I think it's intuitive to use min, max as before,

What point does the readability improve when We change the parameter name?

@yu-ratel
Copy link
Author

yu-ratel commented Jun 1, 2024

Thank you for your reply!

The above two codes are alternatives for modification.

export function clamp(value: number, bound1: number, bound2?: number) {
  if (bound2 == null) {
    return Math.min(value, bound1);
  }

  if (bound2 < bound1) {
    throw new Error('The value of bound2 must be a number greater than bound1.');
  }

  return Math.min(Math.max(value, bound1), bound2);
}

Currently, they are written as bound1 and bound2, and in the code snippet an error is thrown stating that the value of bound2 must be greater than bound1.
I personally thought that using more explicit parameter names like (min, max) or (lowerBound, upperBound) would make it easier to understand from a readability perspective.

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

2 participants