some code in Scene belongs in its subclasses

[pixelzoom]

Related to code review #259. This originally was a REVIEW comment, promoting to an issue.

Here's the relevant code and REVIEW comment thread:

//REVIEW this enum is redundant, use WaterScene, SoundScene, and LightScene constructors
//REVIEW* At the moment, this Enumeration simplifies some logic in `Scene.js` and loading subtype
//REVIEW* constructors would result in a requirejs loop.  Please advise.
/**
 * Scenes can be WATER, SOUND or LIGHT.
 *
 * @author Sam Reid (PhET Interactive Simulations)
 */
define( require => {
  'use strict';

  // modules
  const Enumeration = require( 'PHET_CORE/Enumeration' );
  const waveInterference = require( 'WAVE_INTERFERENCE/waveInterference' );

  return waveInterference.register( 'SceneType', new Enumeration( [ 'WATER', 'SOUND', 'LIGHT' ] ) );
} );

The "requirejs loop" comment made me take a closer look at Scene. The uses of SceneType therein have a code smell to them. This is a case of a base class (Scene) containing code that is specific to subclasses (WaterScene, SoundScene, LightScene). It violates this code-review item:

• Is inheritance used where appropriate? Does the type hierarchy make sense?

One example from Scene, line 180:

      // The first button can trigger a pulse, or continuous wave, depending on the disturbanceTypeProperty
      this.button1PressedProperty.lazyLink( isPressed => {
        if ( config.sceneType !== SceneType.WATER ) {
          if ( isPressed ) {
            this.resetPhase();
          }
          if ( isPressed && this.disturbanceTypeProperty.value === DisturbanceType.PULSE ) {
            this.startPulse();
          }
          else {

            // Water propagates via the water drop
            this.continuousWave1OscillatingProperty.value = isPressed;
          }
        }

        // Clear plane waves if the red button is deselected when paused.
        if ( this.waveSpatialType === WaveSpatialType.PLANE && !isPressed ) {
          this.setSourceValues();
          this.lattice.changedEmitter.emit();
        }
      } );

The if ( config.sceneType !== SceneType.WATER ) {...} bit belongs in WaterScene, not in Scene.

Recommended to address this by (1) eliminating SceneType enum, and (2) moving subclass-specific code to the appropriate subclass. This is unfortunately not a trivial change, but Scene is so fundamental that it's worth doing.

[pixelzoom]

To clarify, and for the sake of discussion...

Here's the anti-pattern used in Scene in its most basic form. The superclass contains code that belongs in the subclasses. And the superclass selects subclass-specific code by using knowledge about what type of subclass the constructed instance (this) is.

class Scene {
  constructor() {
    if ( this instanceof WaveScene ) {
      code that is specific to the Wave scene;
    }
    else if ( this instanceof SoundScene ) {
      code that is specific to the Sound scene;
    }
    else {
      code that is specific to the Light scene;
    }
  }
}

class WaveScene extends Scene { ... }

class SoundScene extends Scene { ... }

class LightScene extends Scene { ... } 

The current implementation of Scene uses this same anti-pattern. But to work around requirejs circularities, it replaces instanceof with a secondary type identifier, the SceneType enum:

class Scene {

  // @param {SceneType} sceneType
  constructor( sceneType ) {
    if ( sceneType === SceneType.WAVE ) {
      code that is specific to the Wave scene;
    }
    else if ( sceneType === SceneType.SOUND ) {
      code that is specific to the Sound scene;
    }
    else {
      code that is specific to the Light scene;
    }
  }
}

class WaveScene extends Scene {
  constructor() {
    super( SceneType.WAVE );
  }
}

class SoundScene extends Scene {
  constructor() {
    super( SceneType.SOUND );
  }
}

class LightScene extends Scene {
  constructor() {
    super( SceneType.LIGHT );
  }
}

I'll be happy to discuss, but I don't know of any good argument for using the above patterns in OO programming.

The 3 most common JS patterns for extending a superclass with subclass-specific function are as follows. Pretty basic OO stuff here, but worth repeating. You may need to use one or all of these to untangle Scene.

(1) All subclasses implement a method that is declared abstract in the superclass.

class Scene {
  constructor() {
    someFunction();
  }

  // @abstract
  someFunction() {
    throw new Error( 'subclass must implement someFunction' );
  }
}

class WaveScene extends Scene {
 
  // @override
  someFunction() {
    code that is specific to the Wave scene;
  }
}

class SoundScene extends Scene {
 
  // @override
  someFunction() {
    code that is specific to the Sound scene;
  }
}

class LightScene extends Scene {
 
  // @override
  someFunction() {
    code that is specific to the Light scene;
  }
}

(2) Subclasses override and extend a concrete method implemented in the superclass. A variation on this is to fully replace the superclass method by not chaining to super.someFunction.

class Scene {
  constructor() {
    someFunction();
  }

  someFunction() {
    code that is relevant to all Scenes;
  }
}

class WaveScene extends Scene {
 
  // @override
  someFunction() {
    super.someFunction();
    code that is specific to the Wave scene;
  }
}

class SoundScene extends Scene {
 
  // @override
  someFunction() {
    super.someFunction();
    code that is specific to the Sound scene;
  }
}

class LightScene extends Scene {
 
  // @override
  someFunction() {
    super.someFunction();
    code that is specific to the Light scene;
  }
}

(3) Subclasses provide functionality via constructor parameters (if functionality is required) or options (if functionality is optional).

class Scene {

  // @param {function} someFunction
  constructor( someFunction ) {
    someFunction();
  }
}

class WaveScene extends Scene {
  constructor() {
    super( () => { code that is specific to the Wave scene; }  );
  }
}

class SoundScene extends Scene {
  constructor() {
    super( () => { code that is specific to the Sound scene; }  );
  }
}

class LightScene extends Scene {
  constructor() {
    super( () => { code that is specific to the Light scene; }  );
  }
}

[samreid]

Good recommendation, proposed fix committed, please review.

[pixelzoom]

Very nice, much easier to grok.

A couple of last nits in Scene:

• setSourceValues can be @private. It's currently @protected but doesn't seem to be used by any subclasses.

• setSourceValues is rather large. What do you think about breaking it into @private methods setPointSourceValues and setPlaneSourceValues, so that it becomes:

setSourceValues() {

  // Get the desired amplitude.  For water, this is set through the desiredAmplitudeProperty.  For other
  // scenes, this is set through the amplitudeProperty.
  const amplitude = this.desiredAmplitudeProperty ? this.desiredAmplitudeProperty.get() : this.amplitudeProperty.get();
  const time = this.timeProperty.value;

  if ( this.waveSpatialType === WaveSpatialTypeEnum.POINT ) {
    this.getPointSourceValues( amplitude, time );
  }
  else {
    this.getPlaneSourceValues( amplitude, time );
  }
}

[samreid]

Good ideas, I've done so in the preceding commit. Please review.

[pixelzoom]

👍 Closing.