phetsims / greenhouse-effect

"Greenhouse Effect" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
4 stars 4 forks source link

Duplicate code #337

Closed pixelzoom closed 11 months ago

pixelzoom commented 11 months ago

For https://github.com/phetsims/greenhouse-effect/issues/331 ...

I used WebStorm Code > Analyze Code > Locate Duplicates, and ignored duplicates in js/micro/. Here are the duplicates that seemed significant.


private energyPacketCrossedAltitude( energyPacket: EMEnergyPacket ): boolean {
  const altitude = this.altitudeProperty.value;
  return ( energyPacket.previousAltitude > altitude && energyPacket.altitude <= altitude ) ||
         ( energyPacket.previousAltitude < altitude && energyPacket.altitude >= altitude );
}
this.downArrow = new ArrowNode(
  boundsRectangle.width / 2,
  boundsRectangle.height / 2,
  boundsRectangle.width / 2,
  boundsRectangle.height / 2,
  options.arrowNodeOptions
);
this.upArrow = new ArrowNode(
  boundsRectangle.width / 2,
  boundsRectangle.height / 2,
  boundsRectangle.width / 2,
  boundsRectangle.height / 2,
  options.arrowNodeOptions
);
const options: PanelOptions = {

  minWidth: width,
  maxWidth: width,
  xMargin: PANEL_MARGIN,
  yMargin: PANEL_MARGIN,
  align: 'center' as const,
  fill: GreenhouseEffectColors.controlPanelBackgroundColorProperty,

  // pdom
  tagName: 'div',
  labelTagName: 'h3',
  labelContent: GreenhouseEffectStrings.infraredStringProperty,

  // phet-io
  tandem: tandem,
  visiblePropertyOptions: {
    phetioFeatured: true
  }
};
/**
 * convenience method for determining whether this is an infrared photon
 */
public get isInfrared(): boolean {
  return this.wavelength === GreenhouseEffectConstants.INFRARED_WAVELENGTH;
}
/**
 * convenience method for determining whether the EM energy contained in this packet is in the visible light range
 */
public get isVisible(): boolean {
  return this.wavelength === GreenhouseEffectConstants.VISIBLE_WAVELENGTH;
}
jbphet commented 11 months ago

I've addressed all items listed above. They all felt pretty good except for the last two, which relate to testing instances of types Photon, Wave, and EMEnergyPacket for whether they are visible or infrared. I thought about trying to use a mixin, but have found these to be tricky and hard to maintain, so I ended up going with a pair of simple utility functions. This particular change didn't feel like a big win, but at least it consolidates it all into one place which would make it easier to change if necessary.

@pixelzoom - feel free to take a look if you see this going by, but I think I've addressed them all, so I'll go ahead and close.

pixelzoom commented 11 months ago

👍🏻 Looks great.