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

Should we create DOT/Range.times or equivalent? #123

Closed
samreid opened this issue Mar 26, 2024 · 3 comments
Closed

Should we create DOT/Range.times or equivalent? #123

samreid opened this issue Mar 26, 2024 · 3 comments
Assignees

Comments

@samreid
Copy link
Member

samreid commented Mar 26, 2024

Discovered in #103, there at least 4 occurrences in the simulation where a Range is used to create a scaled/transformed Range. This is leading to a little bit of duplicated code and an opportunity to add a new feature to Range. Range already has shifted so there is a precedent for this kind of method.

For instance:

const MAX_VOLTAGE_RANGE = new Range( ( MAX_VOLTAGE_PERCENT_RANGE.min / 100 ) * MAX_VOLTAGE, ( MAX_VOLTAGE_PERCENT_RANGE.max / 100 ) * MAX_VOLTAGE );

could become something like:

const MAX_VOLTAGE_RANGE = MAX_VOLTAGE_PERCENT_RANGE.times( MAX_VOLTAGE / 100 );
@pixelzoom
Copy link
Contributor

pixelzoom commented Mar 26, 2024

I see 3 places where a Range times method could be used.

I don't see how Range times could be used here without creating an intermediate Range instance:

    // REVIEW - Consider a Range method for this, see https://github.com/phetsims/faradays-electromagnetic-lab/issues/123
    this.loopRadiusRange = new Range(
      Math.sqrt( ( this.loopAreaPercentProperty.range.min / 100 ) * options.maxLoopArea / Math.PI ),
      Math.sqrt( ( this.loopAreaPercentProperty.range.max / 100 ) * options.maxLoopArea / Math.PI )
    );

@samreid
Copy link
Member Author

samreid commented Mar 26, 2024

Good point! We didn't see the Math.sqrt in that one.

@pixelzoom
Copy link
Contributor

For 3 of the 4 places where I could use Range times, I ended up replacing the computed values with literal values, because they did not need to be computed.

For the loopRadiusRange case above, I added a comment, but didn't otherwise change it. Changing it obfuscates conversion from area to radius: r = sqrt( A / PI ).

Here's the implementation of Range times -- which I did not add, because I didn't need it, and don't have time to add to RangeTests.ts:

  /**
   * Returns new Range instance, created by scaling this Range's min and max by the provided scalar value.
   */
  public times( scalar: number ): Range {
    // eslint-disable-next-line no-html-constructors
    return new Range( scalar * this.min, scalar * this.max );
  }

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

No branches or pull requests

2 participants