issues with config pattern

[pixelzoom]

Related to code review #259.

This is my first encounter with the "config" pattern. Instead of a bunch of constructor parameters or an options hash, this pattern bundles a bunch of required properties into one object, and the associated parameter is typically named config.

This pattern is used in Scene and it's subclasses, and WaveInterferenceTimerNode. Let's have a look at Scene's constructor:

    /**
     * @param {Object} config - see below for required properties
     */
    constructor( config ) {

And the "see below for required properties" is where I have a problem with this. Specifically:

(1) Specifying "see below for required properties" does not absolve the programmer of responsibility for documenting and validating those properties.

(2) "See below" means that I have to search for "config." to find the "required properties". In Scene, that search finds 31 matches. I can't easily see how many "required properties" there actually are, since some of them are used multiple times in the constructor.

(3) The individual "required properties" are not documented, and there are no type expressions. I get lucky with some of them because of assignment, e.g.:

      // @public (read-only) {string} - units for this scene
      this.translatedPositionUnits = config.translatedPositionUnits;

But there are some that aren't documented, and I'm left wondering, e.g.:

config.numberOfSources === 1 ? 0 : config.initialSlitSeparation

(4) There are no assertions to verify whether what is "required" was actually provided.

(5) There are no assertions to verify that what is provided is actually valid.

(6) There's very little in Scene's config that couldn't have been described using a couple of required constructor parameters and options. So I'm left wondering whether this is not a legitimate pattern, or a legitimate pattern that is being abused in this case.

[samreid]

There's very little in Scene's config that couldn't have been described using a couple of required constructor parameters and options.

What would the required parameters be? What would the default values be for options?

What about adding a block of assertions like this near the top of the constructor?

// Option A
assert && assert( config.waveSpatialType, 'waveSpatialType must be supplied' ); // {WaveSpatialTypeEnum}
// ...

What about using this pattern that is exercised in chipper? https://github.com/phetsims/chipper/blob/ef3eeb0c30a8c1e3b964926211e6e62b8376cc18/js/grunt/generateDevelopmentHTML.js#L26-L34

  // Option B
  const {
    stylesheets = '',
    bodystyle = ' style="background-color:black;"', // note the preceding ' ' which is essential
    outputFile = `../${repo}/${repo}_en.html`,
    bodystart = '',
    addedPreloads = [], // none to add
    stripPreloads = [], // none to add
    qualifier = ''
  } = options || {};

If we go with B, how do we require certain values?

I'm not really a fan of this, but mentioning it for completeness:

// Option C
const required = key => {
  assert && assert( config[ key ], `${key} must be supplied` );
};

// @public {WaveSpatialTypeEnum}
this.waveSpatialType = required( 'waveSpatialType' );

Though if phetsims/axon#189 was complete, it would be a great way to validate values.

[pixelzoom]

There are 5 occurrences of @param {Object} config.

One is the view: WaveInterferenceTimerNode. The caller of new WaveInterferenceTimerNode isn't providing any kind of complete configuration Object, it's providing a partial set of TimerNode options. And config is passed to the TimerNode superclass's as its options arg. There's nothing about this that follows the config pattern, so recommended to call WaveInterferenceTimerNode's parameter what it is -- options.

The other four occurrences of config are in the model, in constructors of the Scene class hierarchy: Scene, LightScene, SoundScene, WaterScene. I'm going to take a closer look at them, to answer @samreid's questions: "What would the required parameters be? What would the default values be for options?"

[pixelzoom]

Here are the 23 fields that make up the config param for Scene, in the order that they appear in the constructor.

{WaveSpatialType} waveSpatialType
{string} translatedPositionUnits
{number} waveAreaWidth
{string} graphHorizontalAxisLabel
{number} scaleIndicatorLength
{string} positionUnits
{number} timeScaleFactor
{string} timeUnits
{number} minimumFrequency
{number} maximumFrequency
{number} waveSpeed
{string} verticalAxisTitle
{string} graphTitle
{number} numberOfSources
{number} initialSlitSeparation
{Range} sourceSeparationRange 
{number} waveAreaWidth
{number} initialSlitWidth
{Range} slitWidthRange
{Range} slitSeparationRange
{number} timeScaleString
{number} initialAmplitude
{string} planeWaveGeneratorNodeText

WaterScene does something with initialAmplitude and sourceSeparationRange. The other subclasses (SoundScene, LightScene) do nothing with the config, and just pass it to super.

There are a lot of problems with the "see below" approach to documenting config.

Related fields are not consistently co-located, so you can't tell by what's grouped together like you typically can with options. And names don't always help, because some related fields don't have names that indicate that they are related. E.g. graphHorizontalAxisLabel and verticalAxisTitle are nowhere near each other in the constructor, and you'd never know by their names that they apply to the same graph. (I resorted to console.log to verify.)

None of the config fields are explicitly documented. You have to figure it out by context. The type expressions shown in the list below don't appear in any documentation for the config fields, I teased this info out myself.

It took me ~20 minutes to create this list and write this comment. It would (optimistically) take me another 15-30 minutes to understand all of the fields.

[pixelzoom]

So back to @samreid's "What would the required parameters be? What would the default values be for options?" The answer is: I have no idea. And I have no idea because there's insufficient documentation of the config fields, and I don't have another 15-30 minutes to try to derive the documentation myself.

Similar to your Option A above... The most brute force approach would be to put a minimum of 23 assertions (one for each config field) at the top of the constructor, each having this general form:

// {TypeExpression} describe the config field 
assert && assert( expression to validate the config field, 'some failure message' );

For example:

// {number} width of the visible part of the lattice in the scene's units
assert && assert( typeof config.waveAreaWidth === 'number', 'invalid waveAreaWidth: ' + config.waveAreaWidth );

I don't understand your Option B.

Riffing on Option C, you could do something like this at the top of the constructor:

      if ( assert ) {
        const configFieldNames = [
          'waveSpatialType', // {WaveSpatialType} blah blah blah
          'translatedPositionUnits', // {string} blah blah blah
          ...
        ];
        configFieldNames.forEach( fieldName => {
          assert && assert( config[ fieldName ] !== undefined, 'config is missing field ' + fieldName );
        } );
      }

This pulls all of the field names and documentation together. It verifies that config has all of the required fields, and verification of specific field values could be done after this if needed. I don't particularly like the config[ fieldName ].

And for whatever approach is used... Group related config fields together. Verify that related fields have names that indicate that they're related.

[pixelzoom]

As I mentioned in the first comment, this is my first encounter with the "config" pattern. So I also have a couple of broader concerns that extend beyond Wave Interference.

(1) Which developers are using this pattern and how are they applying it?

(2) In how many places is this pattern used? How many of them have the same problem that we're seeing here in Scene?

[pixelzoom]

Have a look at what @jonathanolson did in area-model-common/Orientation. config looks a lot like options, documented the same as options, but all of the default values are null. Then validation occurs via assertions after the _.extends call. I like this approach very much.

[samreid]

The config pattern in Orientation.js has the advantage that it matches our style for options, but it has the disadvantage that validation, assignment and documentation are scattered about:

We can use the Validator type introduced in phetsims/axon#189 to consolidate config validation/documentation/assignment. I'll commit a change to Wave Interference that leverages Validator to consolidate all usages of Scene.config at the top of the constructor.

[pixelzoom]

The config pattern in Orientation.js has the advantage that it matches our style for options, but it has the disadvantage that validation, assignment and documentation are scattered about:

Just pointing out that options has this same disadvantage, but it hasn't been much (any?) problem, and it's clear.

The options pattern forces all fields to be described together. The usage of Validator in Wave Interference does not. Config fields are co-located only if (1) the config fields are assigned to Scene fields/vars, and (2) those assignments are kept together and not separated by other code. In the current implementation of Scene, I still need to search through the entire constructor to find the config fields, and I still need to mentally assemble a list of what the fields are. Not a superior pattern imo.

(CM edit: fixed typos)

[samreid]

So are you proposing that Scene would begin like so?

class Scene {

  /**
   * @param {Object} config - see below for required properties
   */
  constructor( config ) {

    config = _.extend( {
      waveSpatialType: null, // {WaveSpatialTypeEnum}
      translatedPositionUnits: null, // {string} - units for this scene
      waveAreaWidth: null, // {number} - width of the visible part of the lattice in the scene's units
      graphHorizontalAxisLabel: null, // {string} - text that describes the horizontal spatial axis
      scaleIndicatorLength: null, // {number} - length that depicts indicate relative scale, see LengthScaleIndicatorNode
      positionUnits: null, // {string} - the units (in English and for the PhET-iO data stream)
      timeScaleFactor: null, // {number} - scale factor to convert seconds of wall time to time for the given scene
      timeUnits: null, // {string} - units for time, shown in the timer and optionally top right of the lattice
      verticalAxisTitle: null, // {string} text to show on the vertical axis on the wave-area graph
      graphTitle: null, // {string} - the title to the shown on the wave-area graph
      numberOfSources: null, // {number} - 1 or 2
      waveSpeed: null, // {number}
      timeScaleString: null, // {string} - displayed at the top right of the wave area
      planeWaveGeneratorNodeText: null, // {string} - shown on the PlaneWaveGeneratorNode
      frequencyRange: null, // {Range}
      initialSlitSeparation: null, // {number}
      sourceSeparationRange: null, // {Range}
      initialSlitWidth: null, // {number}
      slitWidthRange: null, // {number}
      slitSeparationRange: null, // {Range}
      initialAmplitude: null // {number}
    }, config );

    // @public {WaveSpatialTypeEnum}
    this.waveSpatialType = Validator.validate( config.waveSpatialType, { validValues: WaveSpatialTypeEnum.VALUES } );

    // @public (read-only) {string} - units for this scene
    this.translatedPositionUnits = Validator.validate( config.translatedPositionUnits, VALID_STRING );

    // @public (read-only) {number} - width of the visible part of the lattice in the scene's units
    this.waveAreaWidth = Validator.validate( config.waveAreaWidth, POSITIVE_NUMBER );

    // @public (read-only) {string} - text that describes the horizontal spatial axis
    this.graphHorizontalAxisLabel = Validator.validate( config.graphHorizontalAxisLabel, VALID_STRING );

    // @public (read-only) {number} - length that depicts indicate relative scale, see LengthScaleIndicatorNode
    this.scaleIndicatorLength = Validator.validate( config.scaleIndicatorLength, POSITIVE_NUMBER );

    // @public (read-only) {string} - the units (in English and for the PhET-iO data stream)
    this.positionUnits = Validator.validate( config.positionUnits, VALID_STRING );

    // @public (read-only) {number} - scale factor to convert seconds of wall time to time for the given scene
    this.timeScaleFactor = Validator.validate( config.timeScaleFactor, POSITIVE_NUMBER );

    // @public (read-only) {string} - units for time, shown in the timer and optionally top right of the lattice
    this.timeUnits = Validator.validate( config.timeUnits, VALID_STRING );

    // @public (read-only) {string} text to show on the vertical axis on the wave-area graph
    this.verticalAxisTitle = Validator.validate( config.verticalAxisTitle, VALID_STRING );

    // @public (read-only) {string} - the title to the shown on the wave-area graph
    this.graphTitle = Validator.validate( config.graphTitle, VALID_STRING );

    // @public (read-only) {number}
    this.numberOfSources = Validator.validate( config.numberOfSources, { validValues: [ 1, 2 ] } );

    // @public (read-only) {number}
    this.waveSpeed = Validator.validate( config.waveSpeed, POSITIVE_NUMBER );

    // @public (read-only) {string} - displayed at the top right of the wave area
    this.timeScaleString = Validator.validate( config.timeScaleString, { valueType: 'string' } );

    // @public (read-only) {string} - shown on the PlaneWaveGeneratorNode
    this.planeWaveGeneratorNodeText = Validator.validate( config.planeWaveGeneratorNodeText, VALID_STRING );

    // These config values are used to create Property instances.
    const frequencyRange = Validator.validate( config.frequencyRange, VALID_RANGE );
    const initialSlitSeparation = Validator.validate( config.initialSlitSeparation, POSITIVE_NUMBER );
    const sourceSeparationRange = Validator.validate( config.sourceSeparationRange, VALID_RANGE );
    const initialSlitWidth = Validator.validate( config.initialSlitWidth, POSITIVE_NUMBER );
    const slitWidthRange = Validator.validate( config.slitWidthRange, VALID_RANGE );
    const slitSeparationRange = Validator.validate( config.slitSeparationRange, VALID_RANGE );
    const initialAmplitude = Validator.validate( config.initialAmplitude, POSITIVE_NUMBER );

    // @public the frequency in the appropriate units for the scene
    this.frequencyProperty = new NumberProperty( frequencyRange.getCenter(), { range: frequencyRange } );

    // @public distance between the sources in the units of the scene, or 0 if there is only one
    // source initialized to match the initial slit separation,
    // see https://github.com/phetsims/wave-interference/issues/87
    this.sourceSeparationProperty = new NumberProperty( initialSlitSeparation, {
      units: this.positionUnits,
      range: sourceSeparationRange
    } );

    // @public - width of the slit(s) opening in the units for this scene
    this.slitWidthProperty = new NumberProperty( initialSlitWidth, {
      units: this.positionUnits,
      range: slitWidthRange
    } );

    // @public distance between the center of the slits, in the units for this scene
    this.slitSeparationProperty = new NumberProperty( initialSlitSeparation, {
      units: this.positionUnits,
      range: slitSeparationRange
    } );

    // @public - controls the amplitude of the wave.
    this.amplitudeProperty = new NumberProperty( initialAmplitude, {
      range: WaveInterferenceConstants.AMPLITUDE_RANGE
    } );

[pixelzoom]

That seems better to me. I'd also like to see related config fields grouped together and potentially renamed (e.g. graph title and axis labels).

[samreid]

I can see how the config = _.extend( { ... },config ) call introduced in #275 (comment) enhances readability and gets all of the config entries to be described together. However, it seems odd to me that it provides no value at runtime. For instance, the code with and without the config = _.extend( { ... },config ) will have exactly the same runtime behavior, except for the code with the config = _.extend( { ... },config ) will take more CPU time, and allocate more objects to the heap and incur a larger file size. To be clear, it is a negligible amount more in each of these dimensions, but it's making me wonder whether we should be writing documentation instead of config = _.extend( { ... },config ). For the sake of discussion, compare to:

/**
 * Config has the following required attributes:
 * waveSpatialType {WaveSpatialTypeEnum}
 * translatedPositionUnits {string} - units for this scene
 * waveAreaWidth {number} - width of the visible part of the lattice in the scene's units
 * graphHorizontalAxisLabel {string} - text that describes the horizontal spatial axis
 * scaleIndicatorLength {number} - length that depicts indicate relative scale, see LengthScaleIndicatorNode
 * positionUnits {string} - the units (in English and for the PhET-iO data stream)
 * timeScaleFactor {number} - scale factor to convert seconds of wall time to time for the given scene
 * timeUnits {string} - units for time, shown in the timer and optionally top right of the lattice
 * verticalAxisTitle {string} text to show on the vertical axis on the wave-area graph
 * graphTitle {string} - the title to the shown on the wave-area graph
 * numberOfSources {number} - 1 or 2
 * waveSpeed {number}
 * timeScaleString {string} - displayed at the top right of the wave area
 * planeWaveGeneratorNodeText {string} - shown on the PlaneWaveGeneratorNode
 * frequencyRange {Range}
 * initialSlitSeparation {number}
 * sourceSeparationRange {Range}
 * initialSlitWidth {number}
 * slitWidthRange {number}
 * slitSeparationRange {Range}
 * initialAmplitude {number}
 */

Is the main reason to stick with _.extend() because of precedent and team familiarity and uniformity with options = _.extend() ?

EDIT: I realized that if this is a code comment, we don't have to put : null everywhere. I'll update the example...

[pixelzoom]

... However, it seems odd to me that it provides no value at runtime ..

I don't understand the foundation of this argument. A program written in assembly language can provide identical behavior to one written in a higher-level language. Which one would you prefer to maintain?

... the code with the config = _.extend( { ... },config ) will take more CPU time, and allocate more objects to the heap and incur a larger file size. To be clear, it is a negligible amount more in each of these dimensions, ...

More CPU time and more object allocations than what, Validator? I haven't taken a look inside Validator yet, but I'm skeptical of that claim.

... but it's making me wonder whether we should be writing documentation instead of config = _.extend( { ... },config ).

Documentation separated from code is typically omitted or suspect -- suspect because it's likely to not have been maintained with the code and therefore not reflect reality.

[samreid]

We are considering the following choices:

// Option A
class Scene {

  /**
   * @param {Object} config - see below for required properties
   */
  constructor( config ) {

    config = _.extend( {
      waveSpatialType: null, // {WaveSpatialTypeEnum}
      translatedPositionUnits: null, // {string} - units for this scene
      waveAreaWidth: null, // {number} - width of the visible part of the lattice in the scene's units
      graphHorizontalAxisLabel: null, // {string} - text that describes the horizontal spatial axis
      scaleIndicatorLength: null, // {number} - length that depicts indicate relative scale, see LengthScaleIndicatorNode
      positionUnits: null, // {string} - the units (in English and for the PhET-iO data stream)
      timeScaleFactor: null, // {number} - scale factor to convert seconds of wall time to time for the given scene
      timeUnits: null, // {string} - units for time, shown in the timer and optionally top right of the lattice
      verticalAxisTitle: null, // {string} text to show on the vertical axis on the wave-area graph
      graphTitle: null, // {string} - the title to the shown on the wave-area graph
      numberOfSources: null, // {number} - 1 or 2
      waveSpeed: null, // {number}
      timeScaleString: null, // {string} - displayed at the top right of the wave area
      planeWaveGeneratorNodeText: null, // {string} - shown on the PlaneWaveGeneratorNode
      frequencyRange: null, // {Range}
      initialSlitSeparation: null, // {number}
      sourceSeparationRange: null, // {Range}
      initialSlitWidth: null, // {number}
      slitWidthRange: null, // {number}
      slitSeparationRange: null, // {Range}
      initialAmplitude: null // {number}
    }, config );

    // @public {WaveSpatialTypeEnum}
    this.waveSpatialType = Validator.validate( config.waveSpatialType, { validValues: WaveSpatialTypeEnum.VALUES } );

    // @public (read-only) {string} - units for this scene
    this.translatedPositionUnits = Validator.validate( config.translatedPositionUnits, VALID_STRING );

    // @public (read-only) {number} - width of the visible part of the lattice in the scene's units
    this.waveAreaWidth = Validator.validate( config.waveAreaWidth, POSITIVE_NUMBER );

    // @public (read-only) {string} - text that describes the horizontal spatial axis
    this.graphHorizontalAxisLabel = Validator.validate( config.graphHorizontalAxisLabel, VALID_STRING );

    // @public (read-only) {number} - length that depicts indicate relative scale, see LengthScaleIndicatorNode
    this.scaleIndicatorLength = Validator.validate( config.scaleIndicatorLength, POSITIVE_NUMBER );

    // @public (read-only) {string} - the units (in English and for the PhET-iO data stream)
    this.positionUnits = Validator.validate( config.positionUnits, VALID_STRING );

    // @public (read-only) {number} - scale factor to convert seconds of wall time to time for the given scene
    this.timeScaleFactor = Validator.validate( config.timeScaleFactor, POSITIVE_NUMBER );

    // @public (read-only) {string} - units for time, shown in the timer and optionally top right of the lattice
    this.timeUnits = Validator.validate( config.timeUnits, VALID_STRING );

    // @public (read-only) {string} text to show on the vertical axis on the wave-area graph
    this.verticalAxisTitle = Validator.validate( config.verticalAxisTitle, VALID_STRING );

    // @public (read-only) {string} - the title to the shown on the wave-area graph
    this.graphTitle = Validator.validate( config.graphTitle, VALID_STRING );

    // @public (read-only) {number}
    this.numberOfSources = Validator.validate( config.numberOfSources, { validValues: [ 1, 2 ] } );

    // @public (read-only) {number}
    this.waveSpeed = Validator.validate( config.waveSpeed, POSITIVE_NUMBER );

    // @public (read-only) {string} - displayed at the top right of the wave area
    this.timeScaleString = Validator.validate( config.timeScaleString, { valueType: 'string' } );

    // @public (read-only) {string} - shown on the PlaneWaveGeneratorNode
    this.planeWaveGeneratorNodeText = Validator.validate( config.planeWaveGeneratorNodeText, VALID_STRING );

    // These config values are used to create Property instances.
    const frequencyRange = Validator.validate( config.frequencyRange, VALID_RANGE );
    const initialSlitSeparation = Validator.validate( config.initialSlitSeparation, POSITIVE_NUMBER );
    const sourceSeparationRange = Validator.validate( config.sourceSeparationRange, VALID_RANGE );
    const initialSlitWidth = Validator.validate( config.initialSlitWidth, POSITIVE_NUMBER );
    const slitWidthRange = Validator.validate( config.slitWidthRange, VALID_RANGE );
    const slitSeparationRange = Validator.validate( config.slitSeparationRange, VALID_RANGE );
    const initialAmplitude = Validator.validate( config.initialAmplitude, POSITIVE_NUMBER );

    // @public the frequency in the appropriate units for the scene
    this.frequencyProperty = new NumberProperty( frequencyRange.getCenter(), { range: frequencyRange } );

    // @public distance between the sources in the units of the scene, or 0 if there is only one
    // source initialized to match the initial slit separation,
    // see https://github.com/phetsims/wave-interference/issues/87
    this.sourceSeparationProperty = new NumberProperty( initialSlitSeparation, {
      units: this.positionUnits,
      range: sourceSeparationRange
    } );

    // @public - width of the slit(s) opening in the units for this scene
    this.slitWidthProperty = new NumberProperty( initialSlitWidth, {
      units: this.positionUnits,
      range: slitWidthRange
    } );

    // @public distance between the center of the slits, in the units for this scene
    this.slitSeparationProperty = new NumberProperty( initialSlitSeparation, {
      units: this.positionUnits,
      range: slitSeparationRange
    } );

    // @public - controls the amplitude of the wave.
    this.amplitudeProperty = new NumberProperty( initialAmplitude, {
      range: WaveInterferenceConstants.AMPLITUDE_RANGE
    } );

And

// Option B
class Scene {

  /**
   * @param {Object} config - see below for required properties
   */
  constructor( config ) {

    // @public {WaveSpatialTypeEnum}
    this.waveSpatialType = Validator.validate( config.waveSpatialType, { validValues: WaveSpatialTypeEnum.VALUES } );

    // @public (read-only) {string} - units for this scene
    this.translatedPositionUnits = Validator.validate( config.translatedPositionUnits, VALID_STRING );

    // @public (read-only) {number} - width of the visible part of the lattice in the scene's units
    this.waveAreaWidth = Validator.validate( config.waveAreaWidth, POSITIVE_NUMBER );

    // @public (read-only) {string} - text that describes the horizontal spatial axis
    this.graphHorizontalAxisLabel = Validator.validate( config.graphHorizontalAxisLabel, VALID_STRING );

    // @public (read-only) {number} - length that depicts indicate relative scale, see LengthScaleIndicatorNode
    this.scaleIndicatorLength = Validator.validate( config.scaleIndicatorLength, POSITIVE_NUMBER );

    // @public (read-only) {string} - the units (in English and for the PhET-iO data stream)
    this.positionUnits = Validator.validate( config.positionUnits, VALID_STRING );

    // @public (read-only) {number} - scale factor to convert seconds of wall time to time for the given scene
    this.timeScaleFactor = Validator.validate( config.timeScaleFactor, POSITIVE_NUMBER );

    // @public (read-only) {string} - units for time, shown in the timer and optionally top right of the lattice
    this.timeUnits = Validator.validate( config.timeUnits, VALID_STRING );

    // @public (read-only) {string} text to show on the vertical axis on the wave-area graph
    this.verticalAxisTitle = Validator.validate( config.verticalAxisTitle, VALID_STRING );

    // @public (read-only) {string} - the title to the shown on the wave-area graph
    this.graphTitle = Validator.validate( config.graphTitle, VALID_STRING );

    // @public (read-only) {number}
    this.numberOfSources = Validator.validate( config.numberOfSources, { validValues: [ 1, 2 ] } );

    // @public (read-only) {number}
    this.waveSpeed = Validator.validate( config.waveSpeed, POSITIVE_NUMBER );

    // @public (read-only) {string} - displayed at the top right of the wave area
    this.timeScaleString = Validator.validate( config.timeScaleString, { valueType: 'string' } );

    // @public (read-only) {string} - shown on the PlaneWaveGeneratorNode
    this.planeWaveGeneratorNodeText = Validator.validate( config.planeWaveGeneratorNodeText, VALID_STRING );

    // These config values are used to create Property instances.
    const frequencyRange = Validator.validate( config.frequencyRange, VALID_RANGE );
    const initialSlitSeparation = Validator.validate( config.initialSlitSeparation, POSITIVE_NUMBER );
    const sourceSeparationRange = Validator.validate( config.sourceSeparationRange, VALID_RANGE );
    const initialSlitWidth = Validator.validate( config.initialSlitWidth, POSITIVE_NUMBER );
    const slitWidthRange = Validator.validate( config.slitWidthRange, VALID_RANGE );
    const slitSeparationRange = Validator.validate( config.slitSeparationRange, VALID_RANGE );
    const initialAmplitude = Validator.validate( config.initialAmplitude, POSITIVE_NUMBER );

    // @public the frequency in the appropriate units for the scene
    this.frequencyProperty = new NumberProperty( frequencyRange.getCenter(), { range: frequencyRange } );

    // @public distance between the sources in the units of the scene, or 0 if there is only one
    // source initialized to match the initial slit separation,
    // see https://github.com/phetsims/wave-interference/issues/87
    this.sourceSeparationProperty = new NumberProperty( initialSlitSeparation, {
      units: this.positionUnits,
      range: sourceSeparationRange
    } );

    // @public - width of the slit(s) opening in the units for this scene
    this.slitWidthProperty = new NumberProperty( initialSlitWidth, {
      units: this.positionUnits,
      range: slitWidthRange
    } );

    // @public distance between the center of the slits, in the units for this scene
    this.slitSeparationProperty = new NumberProperty( initialSlitSeparation, {
      units: this.positionUnits,
      range: slitSeparationRange
    } );

    // @public - controls the amplitude of the wave.
    this.amplitudeProperty = new NumberProperty( initialAmplitude, {
      range: WaveInterferenceConstants.AMPLITUDE_RANGE
    } );

// Option C
class Scene {

  /**
   * @param {Object} config - see below for required properties
   */
  constructor( config ) {

    /**
     * Config has the following required attributes:
     * waveSpatialType {WaveSpatialTypeEnum}
     * translatedPositionUnits {string} - units for this scene
     * waveAreaWidth {number} - width of the visible part of the lattice in the scene's units
     * graphHorizontalAxisLabel {string} - text that describes the horizontal spatial axis
     * scaleIndicatorLength {number} - length that depicts indicate relative scale, see LengthScaleIndicatorNode
     * positionUnits {string} - the units (in English and for the PhET-iO data stream)
     * timeScaleFactor {number} - scale factor to convert seconds of wall time to time for the given scene
     * timeUnits {string} - units for time, shown in the timer and optionally top right of the lattice
     * verticalAxisTitle {string} text to show on the vertical axis on the wave-area graph
     * graphTitle {string} - the title to the shown on the wave-area graph
     * numberOfSources {number} - 1 or 2
     * waveSpeed {number}
     * timeScaleString {string} - displayed at the top right of the wave area
     * planeWaveGeneratorNodeText {string} - shown on the PlaneWaveGeneratorNode
     * frequencyRange {Range}
     * initialSlitSeparation {number}
     * sourceSeparationRange {Range}
     * initialSlitWidth {number}
     * slitWidthRange {number}
     * slitSeparationRange {Range}
     * initialAmplitude {number}
     */    

    // @public {WaveSpatialTypeEnum}
    this.waveSpatialType = Validator.validate( config.waveSpatialType, { validValues: WaveSpatialTypeEnum.VALUES } );

    // @public (read-only) {string} - units for this scene
    this.translatedPositionUnits = Validator.validate( config.translatedPositionUnits, VALID_STRING );

    // @public (read-only) {number} - width of the visible part of the lattice in the scene's units
    this.waveAreaWidth = Validator.validate( config.waveAreaWidth, POSITIVE_NUMBER );

    // @public (read-only) {string} - text that describes the horizontal spatial axis
    this.graphHorizontalAxisLabel = Validator.validate( config.graphHorizontalAxisLabel, VALID_STRING );

    // @public (read-only) {number} - length that depicts indicate relative scale, see LengthScaleIndicatorNode
    this.scaleIndicatorLength = Validator.validate( config.scaleIndicatorLength, POSITIVE_NUMBER );

    // @public (read-only) {string} - the units (in English and for the PhET-iO data stream)
    this.positionUnits = Validator.validate( config.positionUnits, VALID_STRING );

    // @public (read-only) {number} - scale factor to convert seconds of wall time to time for the given scene
    this.timeScaleFactor = Validator.validate( config.timeScaleFactor, POSITIVE_NUMBER );

    // @public (read-only) {string} - units for time, shown in the timer and optionally top right of the lattice
    this.timeUnits = Validator.validate( config.timeUnits, VALID_STRING );

    // @public (read-only) {string} text to show on the vertical axis on the wave-area graph
    this.verticalAxisTitle = Validator.validate( config.verticalAxisTitle, VALID_STRING );

    // @public (read-only) {string} - the title to the shown on the wave-area graph
    this.graphTitle = Validator.validate( config.graphTitle, VALID_STRING );

    // @public (read-only) {number}
    this.numberOfSources = Validator.validate( config.numberOfSources, { validValues: [ 1, 2 ] } );

    // @public (read-only) {number}
    this.waveSpeed = Validator.validate( config.waveSpeed, POSITIVE_NUMBER );

    // @public (read-only) {string} - displayed at the top right of the wave area
    this.timeScaleString = Validator.validate( config.timeScaleString, { valueType: 'string' } );

    // @public (read-only) {string} - shown on the PlaneWaveGeneratorNode
    this.planeWaveGeneratorNodeText = Validator.validate( config.planeWaveGeneratorNodeText, VALID_STRING );

    // These config values are used to create Property instances.
    const frequencyRange = Validator.validate( config.frequencyRange, VALID_RANGE );
    const initialSlitSeparation = Validator.validate( config.initialSlitSeparation, POSITIVE_NUMBER );
    const sourceSeparationRange = Validator.validate( config.sourceSeparationRange, VALID_RANGE );
    const initialSlitWidth = Validator.validate( config.initialSlitWidth, POSITIVE_NUMBER );
    const slitWidthRange = Validator.validate( config.slitWidthRange, VALID_RANGE );
    const slitSeparationRange = Validator.validate( config.slitSeparationRange, VALID_RANGE );
    const initialAmplitude = Validator.validate( config.initialAmplitude, POSITIVE_NUMBER );

    // @public the frequency in the appropriate units for the scene
    this.frequencyProperty = new NumberProperty( frequencyRange.getCenter(), { range: frequencyRange } );

    // @public distance between the sources in the units of the scene, or 0 if there is only one
    // source initialized to match the initial slit separation,
    // see https://github.com/phetsims/wave-interference/issues/87
    this.sourceSeparationProperty = new NumberProperty( initialSlitSeparation, {
      units: this.positionUnits,
      range: sourceSeparationRange
    } );

    // @public - width of the slit(s) opening in the units for this scene
    this.slitWidthProperty = new NumberProperty( initialSlitWidth, {
      units: this.positionUnits,
      range: slitWidthRange
    } );

    // @public distance between the center of the slits, in the units for this scene
    this.slitSeparationProperty = new NumberProperty( initialSlitSeparation, {
      units: this.positionUnits,
      range: slitSeparationRange
    } );

    // @public - controls the amplitude of the wave.
    this.amplitudeProperty = new NumberProperty( initialAmplitude, {
      range: WaveInterferenceConstants.AMPLITUDE_RANGE
    } );

To summarize: Option A has _.extend(), Option B has none and C has a comment. All 3 of these have the same runtime behavior. I'm trying to figure out why you prefer A over B or C. A has 24 more lines of code and much duplicated documentation that now has to be maintained and will likely get out of sync (compared to B). Option C has 24 more lines of code comments that must be maintained and will likely get out of sync (compared to A). Option C is preferable to A because you don't have to repeat null so many times. Option A and C have the advantage that all config parameters are described together.
Option A has the advantage that it matches our precedent, but in this case it just has a lot of code which is not much more useful than documentation.

[samreid]

I'd also like to see related config fields grouped together and potentially renamed (e.g. graph title and axis labels).

Pushed in the preceding commits, please review.

[pixelzoom]

Groups and names of config fields looks good in the above commits. Back to @samreid in case he still wants to discuss options identified in #275 (comment).

[samreid]

I'd like to get feedback from other developers, so I'll mark this for developer meeting. This is a long issue, so I'll summarize briefly here and again during dev meeting.

Please refer to how _.extend({},config) is used in wave-interference/js/common/model/Scene.js and area-model-common/js/common/model/Orientation.js. In each case, the options object literal has all null values is more meant for developers + readability than for the computer. For example, in Scene.js, the code would have the same runtime behavior if the 33 lines beginning with config = _.extend( { was deleted. Normally I would say it is a good idea to eliminate 33 lines of code that does nothing, but this code was requested during code review for matching precedent with our options pattern. I know we have a huge investment in the options pattern, but I'm interested in discussing a pattern that consolidates (default value + validation + assignment) instead of requiring them to be scattered.

[samreid]

We discussed this at developer meeting today. Here are some raw notes. Leaving self assigned to add more reflection before we move on:

JO: would be nice to quickly scan a file and see the ...
AccordionBox is quickly scannable and readable.

But maybe this belongs in documentation, not in a _.extends call.

It's too difficult to look into Validator.validate lines to see what all eth options are.

Concern about performance and memory.

It's nice to have documentation + validation + default + assignment together.
JO: I agree those are nice qualities too.
JB: It's nice to have those together, and I would get used to the alternative pattern. We should not just use the prior pattern because we are used to it.
JG: I agree with JB
CK: The proposed pattern seems readable.
JG: I would need some time to get used to the new pattern.
JG: Would this work with our options nesting pattern?
SR: It seem it would be the same with either.

[samreid]

What about when we need to call super(options) with an extended options? In that case, we need to call _.extend({},options) anyways, right?

[samreid]

I'll move the discussion about Validator and options to axon (where Validator lives), for more general discussion. @pixelzoom can this issue be closed?

[pixelzoom]

Yep. Closing.