From 2e976d27953b6eb1e5dfd18745282c0368123ede Mon Sep 17 00:00:00 2001
From: Marla Schulz <marlaa.schulz@gmail.com>
Date: Fri, 21 Oct 2022 15:23:04 -0600
Subject: [PATCH] Review and formatting, see:
 https://github.com/phetsims/sun/issues/792

---
 js/CompletePiecewiseLinearFunction.ts | 15 ++++++++++-----
 js/Range.ts                           | 11 +++++++++++
 js/RangeWithValue.ts                  |  2 ++
 3 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/js/CompletePiecewiseLinearFunction.ts b/js/CompletePiecewiseLinearFunction.ts
index c909bf8..0c23e93 100644
--- a/js/CompletePiecewiseLinearFunction.ts
+++ b/js/CompletePiecewiseLinearFunction.ts
@@ -9,6 +9,10 @@
  *
  * If a single point is provided, it represents a constant function.
  *
+ * REVIEW: The usage of "this" and "other" is confusing throughout this documentation. Would recommend finding a better way to distinct
+ * between the two. It's a big leap each time to remind myself what "this" and "other" are pointing to.
+ * perhaps "this" = working || base function, "other" = passed in || argument function?
+ *
  * @author Jonathan Olson <jonathan.olson@colorado.edu>
  */
 
@@ -86,7 +90,7 @@ class CompletePiecewiseLinearFunction {
   }
 
   /**
-   * Returns the sorted unique x-values included in either this function or the other function.
+   * Returns an array that combines sorted unique x-values provided by this function and/or the other function.
    */
   private getCombinedXValues( other: CompletePiecewiseLinearFunction ): number[] {
     return CompletePiecewiseLinearFunction.sortedUniqueEpsilon(
@@ -95,8 +99,8 @@ class CompletePiecewiseLinearFunction {
   }
 
   /**
-   * Returns the sorted unique x-values either included in either this function or the other function, OR that result
-   * from the intersection of the two functions.
+   * Returns an array that combines the sorted unique x-values included in this function and/or the other function, OR the unique x-values
+   * that result from the intersection of the two functions.
    */
   private getIntersectedXValues( other: CompletePiecewiseLinearFunction ): number[] {
     const xValues = this.getCombinedXValues( other );
@@ -106,11 +110,12 @@ class CompletePiecewiseLinearFunction {
       const leftX = xValues[ i ];
       const rightX = xValues[ i + 1 ];
       const intersectionPoint = Utils.lineLineIntersection(
-        // Our line
+
+        // The linear function defined in this
         new Vector2( leftX, this.evaluate( leftX ) ),
         new Vector2( rightX, this.evaluate( rightX ) ),
 
-        // Other line
+        // The passed in argument linear function
         new Vector2( leftX, other.evaluate( leftX ) ),
         new Vector2( rightX, other.evaluate( rightX ) )
       );
diff --git a/js/Range.ts b/js/Range.ts
index dcd4a93..b352453 100644
--- a/js/Range.ts
+++ b/js/Range.ts
@@ -97,6 +97,7 @@ class Range implements TRange {
     this._min = min;
     this._max = max;
 
+    // REVIEW: It seems strange to have a return value in a setter...
     return this;
   }
 
@@ -151,6 +152,12 @@ class Range implements TRange {
   }
 
   /**
+   * REVIEW: I think I'm not a fan of using "this" as a class instance reference in documentation in general. The documentation
+   * for this function provides a good example of that. "This" is used both to refer to 'this' class instance, as well
+   * as being used to refer to the union function.
+   *
+   * REVIEW: The naming is not helping me understand that this function is just the immutable version of includeRange().
+   *
    * The smallest range that contains both this range and the input range, returned as a copy.
    *
    * This is the immutable form of the function includeRange(). This will return a new range, and will not modify
@@ -164,6 +171,8 @@ class Range implements TRange {
   }
 
   /**
+   * REVIEW: The naming is not helping me understand that this function is just the immutable version of constrainRange().
+   *
    * The smallest range that is contained by both this range and the input range, returned as a copy.
    *
    * This is the immutable form of the function constrainRange(). This will return a new range, and will not modify
@@ -203,6 +212,8 @@ class Range implements TRange {
   }
 
   /**
+   * REVIEW: do we also need a mutable form of shifted?
+   *
    * Returns a new range that is the same as this range, but shifted by the specified amount.
    */
   public shifted( n: number ): Range {
diff --git a/js/RangeWithValue.ts b/js/RangeWithValue.ts
index 485a126..d39b0aa 100644
--- a/js/RangeWithValue.ts
+++ b/js/RangeWithValue.ts
@@ -64,6 +64,8 @@ class RangeWithValue extends Range {
   public override setMinMax( min: number, max: number ): this {
     assert && assert( this._defaultValue >= min, `min must be <= defaultValue: ${min}` );
     assert && assert( this._defaultValue <= max, `max must be >= defaultValue: ${max}` );
+
+    // REVIEW: Same as setMinMax in Range.ts, returning a value in a setter seems odd...
     return super.setMinMax( min, max );
   }