Skip to content

Commit

Permalink
Review comments, see: #268
Browse files Browse the repository at this point in the history
  • Loading branch information
marlitas committed Mar 14, 2023
1 parent 850e320 commit 2a5a3b8
Show file tree
Hide file tree
Showing 4 changed files with 8 additions and 1 deletion.
1 change: 1 addition & 0 deletions js/common/model/CurvePoint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ export default class CurvePoint {
*/
public undo(): void {

// REVIEW: Why revert to initial state when we reach the undo limit? This seems strange to me https://github.com/phetsims/calculus-grapher/issues/287
// Reverts the state of this CurvedPoint to its most-recently saved state (if available).
// As a side effect, the state is popped off undoStack.
const pointState = ( this.undoStack.length === 0 ) ? this.initialState : this.undoStack.pop()!;
Expand Down
3 changes: 3 additions & 0 deletions js/common/model/DerivativeCurve.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,9 @@ export default class DerivativeCurve extends Curve {
}

if ( typeof leftSlope === 'number' && typeof rightSlope === 'number' ) {

// REVIEW: What does approximately equal mean? I see no analysis of how equal the slopes are before setting the
// REVIEW: derivative.
// If both the left and right adjacent Points of the Point of the 'base' curve exist, the derivative is
// the average of the slopes if they are approximately equal. Otherwise, the derivative doesn't exist.
this.points[ index ].y = ( leftSlope + rightSlope ) / 2;
Expand Down
2 changes: 2 additions & 0 deletions js/common/model/SecondDerivativeCurve.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ export default class SecondDerivativeCurve extends Curve {
}

/**
* // REVIEW: This makes it sound like we're grabbing the derivative of the "base"/"original"
* // REVIEW: curve. But we're actually grabbing the derivative of the OG curve derivative.
* Updates the y-values of the SecondDerivative to represent the derivative of the 'base' Curve.
*
* To update the second derivative, we (1) assume that the points are smooth,
Expand Down
3 changes: 2 additions & 1 deletion js/common/model/TransformedCurve.ts
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ export default class TransformedCurve extends Curve {
const deltaY = y - this.getClosestPointAt( x ).y;

// Shift each of the CurvePoints by deltaY.
this.points.forEach( point => {point.y += deltaY;} );
this.points.forEach( point => { point.y += deltaY; } );
}

/**
Expand Down Expand Up @@ -560,6 +560,7 @@ export default class TransformedCurve extends Curve {
// Point associated with the last drag event
const nextToLastPoint = this.getClosestPointAt( antepenultimatePosition.x );

// REVIEW: should this be "lastPoint is in between closestPoint and nextToLastPoint"?
// Checks that lastPoint is in between closestPoint and lastPoint
if ( ( closestPoint.x - lastPoint.x ) * ( nextToLastPoint.x - lastPoint.x ) < 0 ) {

Expand Down

0 comments on commit 2a5a3b8

Please sign in to comment.