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

Address duplicated code #101

Closed
pixelzoom opened this issue Mar 13, 2024 · 1 comment
Closed

Address duplicated code #101

pixelzoom opened this issue Mar 13, 2024 · 1 comment
Assignees

Comments

@pixelzoom
Copy link
Contributor

Re this CRC item:

I used WebStorm's Locate Duplicates feature, and addressed duplicates that I thought were significant. This issue documents duplicates that I did not address that might be controversial.

(1) ElectromagnetScreenView, TransformerScreenView, and GeneratorScreenView have the same pdomOrder for the control area, and that results in the bit of duplicated code shown below. BarMagnetScreenView and PickupCoilScreenView have a similar relationship. At the ScreenView level, I feel that it's more readable and more maintainable to explicitly show the complete pdomOrder for both play area and control area. So I'm not only fine with this bit of duplication, I prefer it.

    this.pdomControlAreaNode.pdomOrder = [
      panels.toolsPanel,
      timeControlNode,
      this.resetAllButton
    ];

(2) FELSonifier, FieldMeterDragSonifier, CompassSonifier contain some duplicated code. This is experimental sonification code (documented as such), not included in the production version, likely to evolve in a future release. So the duplicatation here does not concern me.

(3) Layout of pickupCoilDebuggerPanel is duplicated in the 3 ScreenView subclasses that involve a pickup coil. This is a debugging tool, not part of the production UI, and I don't feel the need to factor this out.

    pickupCoilDebuggerPanel.centerX = this.layoutBounds.centerX;
    pickupCoilDebuggerPanel.top = this.layoutBounds.top + FELConstants.SCREEN_VIEW_Y_MARGIN;

(4) BarMagnetScreenView and PickupCoilScreenView both use the same type of compass and do not have time controls. The code below is mostly duplicated, except that the compass is not visible in the Pickup Coil screen. At the ScreenView level, I don't feel the need to complicate things by factoring this out.

    super( barMagnet, {
      createCompass: ( magnet, isPlayingProperty, tandem ) => new KinematicCompass( magnet, isPlayingProperty, {
        position: new Vector2( 625, 400 ),
-       visible: false,
        tandem: tandem
      } ),
      isPlayingPropertyOptions: {
        tandem: Tandem.OPT_OUT // because this screen has no time controls
      },
      tandem: tandem
    } );

(5) PickupCoilScreenView and TransformerScreenView have the "Lock to Axis" feature, and therefore the isLockedToAxisProperty shown below. How isLockedToAxisProperty is subsequently used by these screens is very different. So I don't feel a need to factor this out.

    const isLockedToAxisProperty = new BooleanProperty( false, {
      tandem: tandem.createTandem( 'isLockedToAxisProperty' ),
      phetioDocumentation: 'When true, dragging the magnet or pickup coil is locked to the pickup coil\'s horizontal axis.'
    } );
@pixelzoom
Copy link
Contributor Author

I address duplicated code that I thought was significant in the above commits. Closing.

@pixelzoom pixelzoom mentioned this issue Mar 13, 2024
89 tasks
pixelzoom added a commit that referenced this issue Mar 13, 2024
pixelzoom added a commit to phetsims/magnets-and-electromagnets that referenced this issue Mar 13, 2024
pixelzoom added a commit to phetsims/magnet-and-compass that referenced this issue Mar 13, 2024
pixelzoom added a commit to phetsims/generator that referenced this issue Mar 13, 2024
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

1 participant