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

Duplicate code #337

Closed
5 of 6 tasks
pixelzoom opened this issue Jul 13, 2023 · 2 comments
Closed
5 of 6 tasks

Duplicate code #337

pixelzoom opened this issue Jul 13, 2023 · 2 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Jul 13, 2023

For #331 ...

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


  • energyPacketCrossedAltitude in FluxSensor.ts and EnergyAbsorbingEmittingLayer.ts
private energyPacketCrossedAltitude( energyPacket: EMEnergyPacket ): boolean {
  const altitude = this.altitudeProperty.value;
  return ( energyPacket.previousAltitude > altitude && energyPacket.altitude <= altitude ) ||
         ( energyPacket.previousAltitude < altitude && energyPacket.altitude >= altitude );
}
  • new ArrowNode in FluxMeterNode.ts:
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 in InfraRedPanel.ts and SunlightPanel.ts
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
  }
};
  • isInfrared in Photon.ts, EMEnergyPacket.ts, Wave.ts
/**
 * convenience method for determining whether this is an infrared photon
 */
public get isInfrared(): boolean {
  return this.wavelength === GreenhouseEffectConstants.INFRARED_WAVELENGTH;
}
  • isVisible in Photon.ts, EMEnergyPacket.ts, Wave.ts
/**
 * 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
Copy link
Contributor

jbphet commented Jul 21, 2023

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.

@jbphet jbphet closed this as completed Jul 21, 2023
@pixelzoom
Copy link
Contributor Author

👍🏻 Looks great.

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