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

insufficient documentation for calls to link, addListener, etc. #156

Closed pixelzoom closed 5 years ago

pixelzoom commented 5 years ago

For this item in code review #143:

  • [ ] Are there leaks due to registering observers or listeners? The following guidelines should be followed unless there it is obviously no need to unlink, or documentation (in-line or in the implementation nodes)added about why following them is not necessary. Unlink is not needed for properties contained in classes that are never disposed of, such as primary model and view classes that exist for the duration of the sim.
  • [ ] AXON: Property.link is accompanied by Property.unlink.
  • [ ] AXON: Creation of DerivedProperty is accompanied by dispose.
  • [ ] AXON: Creation of Multilink is accompanied by dispose.
  • [ ] AXON: Events.on is accompanied by Events.off.
  • [ ] AXON: Emitter.addListener is accompanied by Emitter.removeListener.
  • [ ] SCENERY: Node.on is accompanied by Node.off
  • [ ] TANDEM: PhET-iO instrumented PhetioObject instances should be disposed.

I inspected all occurrences of "link(". Sometimes there's a comment, e.g. in CurveFittingModel:

      // These are unlinked in removePoint
      point.positionProperty.link( this.updateCurveFit );
      point.isInsideGraphProperty.link( this.updateCurveFit );
      point.deltaProperty.link( this.updateCurveFit );

But many times there's no information about whether unlink is needed or not, or why. E.g. in CurveFittingModel:

      // validate Property values and update curve fit
      this.orderProperty.link( order => {

        // ensure the order is 1, 2 or 3: linear, quadratic or cubic
        assert && assert( order === 1 || order === 2 || order === 3, `invalid order: ${order}` );
        this.updateCurveFit();
      } );
      this.fitProperty.link( this.updateCurveFit );

There's also no description of the sim's general policy in implementation-notes.md, which would make such comments unnecessary when they conform to that policy. E.g. something like "all Properties exist for the lifetime of the sim and don't need to be unlinked unless otherwise noted in code comments".

I also inspected DerivedProperty instantiations, and there's no info about whether they need to be dispose and why.

Ditto for Emitter addListener.

So there's some work to be done here, documenting which registrations need or don't need to be unregistered.

SaurabhTotey commented 5 years ago

I tried finding all missing comments, but it is possible that I may have missed a few. Assigning to @pixelzoom to review.

pixelzoom commented 5 years ago

@SaurabhTotey I found a few more in the above commit. Feel free to close after reviewing.

SaurabhTotey commented 5 years ago

It all seems good to me. Closing.