phetsims / curve-fitting

"Curve Fitting" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 3 forks source link

unused dispose functions #158

Closed pixelzoom closed 5 years ago

pixelzoom commented 5 years ago

Related to this item for code review #143:

  • [ ] Do all types that require a dispose function have one? This should expose a public dispose function that calls this.disposeMyType(), where disposeMyType is a private function declared in the constructor. MyType should exactly match the filename.

Implementing dispose functions that are not used is also a problem. It's unexercised code, and it confuses future maintainers about what really needs to be disposed. That said...

I see implementations of dispose for BarometerNode, BucketNode, EquationNode, PointNode. But the only call to dispose that I see being used is for PointNode, in BucketNode:

        const removalListener = removedPoint => {
          if ( removedPoint === addedPoint ) {
            this.removeChild( pointNode );
            pointNode.dispose();
            points.removeItemRemovedListener( removalListener );
          }
        };

If the dispose implementations for BarometerNode, BucketNode, and EquationNode are really not needed, then I recommend deleting them.

I also recommend putting a note in implementation-notes.md about what needs to be disposed any why.

SaurabhTotey commented 5 years ago

I removed the dispose functions for BarometerNode and EquationNode, but I was unable to find BucketNode's dispose. Did I miss something, or is this all good now?

pixelzoom commented 5 years ago

Looks like you got them all @SaurabhTotey. I must've been hallucinating about BucketNode, or might have been looking at FRACTIONS_COMMON/BucketNode.

Closing.