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

duplicated code in LatticeCanvasNode and LightScreenNode #277

Closed
pixelzoom opened this issue Dec 19, 2018 · 5 comments
Closed

duplicated code in LatticeCanvasNode and LightScreenNode #277

pixelzoom opened this issue Dec 19, 2018 · 5 comments

Comments

@pixelzoom
Copy link
Contributor

For code review #259, identified by grunt find-duplicates.

The code below is duplicated in LatticeCanvasNode.js (lines 51-68) and LightScreenNode.js (lines 59-76).

      const height = this.lattice.height - this.lattice.dampY * 2;

      // @private
      this.directCanvas = document.createElement( 'canvas' );
      this.directCanvas.width = width;
      this.directCanvas.height = height;

      // @private
      this.directContext = this.directCanvas.getContext( '2d' );

      // @private
      this.imageData = this.directContext.createImageData( width, height );

      // Invalidate paint when model indicates changes
      const invalidateSelfListener = this.invalidatePaint.bind( this );
      lattice.changedEmitter.addListener( invalidateSelfListener );
@pixelzoom pixelzoom changed the title duplicated code duplicated code in LatticeCanvasNode and LightScreenNode Dec 19, 2018
@samreid samreid mentioned this issue Dec 19, 2018
samreid added a commit that referenced this issue Dec 21, 2018
@samreid
Copy link
Member

samreid commented Dec 21, 2018

Proposed fix pushed, please review.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Dec 21, 2018

ImageDataRenderer looks very nice, could be useful in common code someday (maybe even in Gas Properties). With that in mind...

The first 2 lines of the constructor are:

constructor( lattice, width ) {
 const height = lattice.height - lattice.dampY * 2;

... and the lattice is never used again. How about changing to:

constructor( width, height ) {

... with call sites like:

  this.imageDataRenderer = new ImageDataRenderer( width, this.lattice.height - this.lattice.dampY * 2 );

And perhaps move the this.lattice.height - this.lattice.dampY * 2 computation into method of Lattice, so that it doesn't need to be duplicated at new ImageDataRenderer call sites.

@samreid
Copy link
Member

samreid commented Dec 22, 2018

Nice recommendation, thanks! I pushed one commit, please review.

@samreid samreid assigned pixelzoom and unassigned samreid Dec 22, 2018
samreid added a commit that referenced this issue Dec 22, 2018
@samreid
Copy link
Member

samreid commented Dec 22, 2018

Oops, previous commit had bad jsdoc params, fixed in an additional commit.

@pixelzoom
Copy link
Contributor Author

👍 Closing.

samreid added a commit to phetsims/scenery-phet that referenced this issue Oct 29, 2022
samreid added a commit to phetsims/scenery-phet that referenced this issue Oct 29, 2022
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