Required options/named parameters

[samreid]

In a few places in the Area Model review phetsims/area-model-common#99 I noticed that options was being used for a named parameter pattern instead of a pure "options" pattern (where values are truly optional and have reasonable defaults). I have had the need to do something like this myself in a few places. I'm wondering if we should:

(a) broaden our definition of "options" to include required named parameters
(b) forbid required arguments in options and move them to required/unnamed parameters
(c) come up with a new argument name for supplying named parameters. Or a strategy for coming up with custom names depending on the circumstances. Would it be parallel to options (with options still being last argument)?
(d) something else?

This does not need to be settled before Area Model review can proceed or before Area Model can be published, but it has come up enough times that I'd like to discuss it.

[samreid]

Also from the code review:

  /**
   * @constructor
   * @extends {Object}
   *
   * @param {Object} options - Has the following fields:
   * @param {Array.<EntryType>} options.horizontal
   * @param {Array.<EntryType>} options.vertical
   * @param {Array.<Array.<EntryType>>} options.products
   * @param {EntryType} options.total
   * @param {EntryType} options.horizontalTotal
   * @param {EntryType} options.verticalTotal
   * @param {AreaChallengeType} options.type
   */
  // REVIEW: This is unconventional and potentially incorrect way to document options.
  // REVIEW: @zepumph said he normally solves this problem with a jsdoc typedef
  // REVIEW*: A typedef that is used once always feels like something that is wrong and should be inlined. Can we
  // REVIEW*: discuss and investigate options for documenting this?
  // REVIEW: This seems related to https://github.com/phetsims/tasks/issues/930
  // REVIEW: I'll add a note about it there
  function AreaChallengeDescription( options ) {

[zepumph]

I would be fine with required params in options as long as the documentation was solid. I worry about inconsistencies about how we document objects playing into the effectiveness of communicating that a parameter is required for the type.

Let's say we settle on the above documentation as the recommended approach for documenting options (not sure it is), then I would like to see something like:
* @param {EntryType} options.horizontalTotal - REQUIRED

[jbphet]

IMO, options should be optional. I've coded all my sims that way and that is my expectation when looking at other developer's sims, and it will increase the difficulty of interpreting other developers' code if we don't all follow this convention. If there are parameters that are required, I think they should either be separate individual arguments if there are only a few, or if grouping them together is easier, there could be a "parameters" or "config" object.

[samreid]

To provide examples of the preceding comment:

new Person1({
 name: 'larry', // name is required
 age: 45, // Age is required, so it appears in the a "config" parameter
},{
 hairColor: 'blue' //hair color is optional
});

function Person1(config,options){
  assert && assert(config.name,'name must be supplied');
  assert && assert(config.name,'name must be supplied');
  this.name =config.name;
  this.age = config.age;
  options = _.extend({color:'black'},options);
  this.hairColor = options.hairColor;
}

new Person2({
 name: 'larry', 
 age: 45, 
 hairColor: 'blue' 
});

function Person2(options){
  assert && assert(options.name,'name must be supplied');
  assert && assert(options.name,'name must be supplied');
  this.name =config.name;
  this.age = config.age;
  options = _.extend({color:'black'},options);
  this.hairColor = options.hairColor;
}

[jonathanolson]

or if grouping them together is easier, there could be a "parameters" or "config" object.

I'm totally fine with config for required things and options for optional things. I just want a nice clean pattern to use when we would have a mess of listed parameters that is hard to read at the call site otherwise.

[pixelzoom]

My 2 cents:

(a) broaden our definition of "options" to include required named parameters

No.

(b) forbid required arguments in options and move them to required/unnamed parameters

Yes

(c) come up with a new argument name for supplying named parameters. Or a strategy for coming up with custom names depending on the circumstances. Would it be parallel to options (with options still being last argument)?

No. If you have something that complex, you probably need a new type, or at least a data structure type. (config may fit into that hole.)

(d) something else?

No. Keep it simple. Named parameters are required. Options are optional, and default values are provided for anything that is optional. If your argument list gets too long, consider a data structure type, or live with it.

[jonathanolson]

No. If you have something that complex, you probably need a new type, or at least a data structure type. (config may fit into that hole.)

The original example is AreaChallengeDescription, where descriptions are created like:

AreaChallengeDescription.LEVEL_1_NUMBERS_1 = new AreaChallengeDescription( {
  horizontal: [ GIVEN, GIVEN ],
  vertical: [ GIVEN ],
  products: [
    [ GIVEN, GIVEN ]
  ],
  total: EDITABLE,
  horizontalTotal: GIVEN,
  verticalTotal: GIVEN,
  type: AreaChallengeType.NUMBERS
} );

and you are saying we would prefer:

AreaChallengeDescription.LEVEL_1_NUMBERS_1 = new AreaChallengeDescription(
  [ GIVEN, GIVEN ],
  [ GIVEN ],
  [ [ GIVEN, GIVEN ] ],
  EDITABLE,
  GIVEN,
  GIVEN,
  AreaChallengeType.NUMBERS
);

which is WAY harder to read. And if it's that complex, I don't know what you mean by a new type, because AreaChallengeDescription is meant to be an object with those properties. It IS the data structure type.

So, do people actually prefer the second one without names? People want to have to look up what the 6th parameter is?

If you are recommending something different, can you please show exactly how the client usage would look here?

[samreid]

In my opinion, inventing another "named, required parameters" object that could be supplied in parallel to options sounds horrible. Let's say I have a complex type that has 3 required unnamed parameters, and an options object which takes 14 options. If I want to make one of the options required but named, it seems odd to take it out of the options object and create a different "config" object to put it in. How would that simplify the implementation or API at all?

[jonathanolson]

So... what if we take everything named options right now and name it config? Does that solve the problem (since some configuration fields may be required, and some may be optional)?

[jonathanolson]

Orientation.js is also a place that would also use this pattern, with all required fields, e.g.:

  {
    coordinate: 'x',
    centerCoordinate: 'centerX',
    modelViewName: 'modelToViewX',
    minSide: 'left',
    maxSide: 'right',
    rectCoordinate: 'rectX',
    rectSize: 'rectWidth',
    layoutBoxOrientation: 'horizontal'
  }

[jessegreenberg]

I don't know what you mean by a new type

For AreaChallengeDescription I was imagining something like this:

  /**
   * @param {DescriptionConfig} descriptionConfig - see required properties in DescriptionConfig
   * @param {options} options
   */
  function AreaChallengeDescription( descriptionConfig, options ) {

    options = _.extend( {
      shufflable: true,
      unique: true
    }, options );

    // assign rest from descriptionConfig

I would prefer this to an object that can have both required and optional fields, even if it is renamed to something other than "options".

[zepumph]

Then this passes the burden onto DescriptionConfig, how do you create that type? One object with named, required params, or individual named args. @jonathanolson's qualms in #930 (comment) still apply.

[pixelzoom]

@jonathanolson asked:

... and you are saying we would prefer:

No. Using an object literal data structure (sin the current implementation of AreaChallengeDescription) is fine.

[pixelzoom]

I think @jessegreenberg has the right idea. Keep optional things in options. Bundle (related!) required things in config. But config doesn't need to be a type, it can be a literal (duck type).

[zepumph]

I like the idea of using {Object} options param for optional and using {Object} config for required args (options still always last). But it sounds like there was some push back on that. It is probably a bit confusing to have both.

[jonathanolson]

I like the idea of using {Object} options param for optional and using {Object} config for required args (options still always last).

That also sounds inconvenient if we need to have a separate nested config object alongside a nested options object (for components that things through). Maybe that can be avoided?

[zepumph]

Brainstorming, what about having a reserved key in the options object called required, to nest required args under. Weird usage though:

options.required.myParam

[pixelzoom]

I'm fine with mixing required and optional fields in config, as long as they are well-documented, and defaults are provided for optional fields using _.extends (ala options). I am not OK with putting required fields in options. And options should always be the last method parameter.

[jonathanolson]

Current proposal:

• We can use the "options" pattern for things are required. When this happens, it should NOT be called options, but should generally be called config.
• Any required fields in a config should have assertions making sure they are present.
• The same nested "config" pattern would be used (like we do for options).

Uncertain:

• Some prefer calling functions like f( config, options ) where optional fields are separated from required fields.
• How do we document required fields? Do we specify a null value inside the _.extend?
• Come up with an exemplar for this pattern, and put it in the code review checklist.

[jonathanolson]

Leaving dev meeting tag for more discussion. @jessegreenberg, can you provide feedback in advance of the meeting?

[jessegreenberg]

I think it could be confusing to have optional fields in config. Why wouldn't these be in options?

How do we document required fields?

We won't have to worry about this if we put optional fields in options. I could learn to look through config to learn which fields are optional and required, but that changes how we previously used options. I don't understand the benefit of changing yet.

[jonathanolson]

Examples for config only (One), or config and options (Two):

function One( config ) {
  config = _.extend( {
    requiredA: value,
    requiredB: value,
    nestedConfig: null

    // optional
    optionalA: value,
    optionalB: value
  }, config );

  config.nestedConfig = _.extend( {
    requiredC: value,

    // optional
    optionalC: value
  }, config.nestedConfig );
}

function Two( config, options ) {
  config = _.extend( {
    requiredA: value,
    requiredB: value,

    nestedConfig: null
  }, config );

  config.nestedConfig = _.extend( {
    requiredC: value
  }, config.nestedConfig );

  options = _.extend( {
    optionalA: value,
    optionalB: value,

    nestedOptions: null
  }, options );

  options.nestedOptions = _.extend( {
    optionalC: value
  }, options.nestedOptions );
}

new One( {
  requiredA: value,
  requiredB: value,
  optionalA: value,
  optionalB: value,
  nestedConfig: {
    requiredC: value,
    optionalC: value
  }
} );

new Two( {
  requiredA: value,
  requiredB: value,
  nestedConfig: {
    requiredC: value
  }
},
{
  optionalA: value,
  optionalB: value,
  nestedOptions: {
    optionalC: value
  }
} );

Things get more complicated if you want to require some things (like a font:) but pass them to e.g. a mutate. Then you need this.mutate( _.extend( {}, config, options ) )?

Is the inconvenience of having twice the _.extend calls, twice the number of input parameter objects, etc. worth the potential confusion from having optional fields in config? I'd say it's more confusing with the split, and how the parameters that were in one place are now spread into different places.

If we move certain things from being options to being required, are we prepared to spend the refactoring cost to change all usages? When everything is phet-io instrumented, are we going to have:

new RectangularPushButton( {
  tandem: tandem.createTandem( 'button' )
}, {
  // options here
} );

and take on the cost of changing core APIs that didn't previously have a config/options split and change the type signature to add config?

[samreid]

I think @jonathanolson and I are on the same wavelength about this. To me the most significant factor is readability of the call site API. new One(...) in the preceding comment looks great--client wants to specify values for the constructor and can do so in a uniform way. new Two(...) seems complicated and confusing.

[pixelzoom]

The new One(...) examples are fine with me. But 2 questions:

(1) How do you propose to document which properties are required vs optional? @param {Type} requiredParamName vs @param {Type} [optionalParamName] clearly communicates this for function parameters. But that documentation style doesn't apply well to config.

(2) Do you intend to provide assertions for all config properties that are required? I think this should be a requirement for using this pattern.

(3) Do you propose to filter out subtype-specific properties from config before propagating to the supertype via its options parameter? Again, I think this should be a requirement for using this pattern.

[pixelzoom]

Re (3)... I anticipate that the argument for not filtering will be "we don't do it for options now, why should we do it for config?" I'm not buying that argument. And I propose that we should "filter before propagating" for both config and options going forward.

[jessegreenberg]

I reviewed comments since #930 (comment). Everyone is in agreement about config, and I am OK with that moving forward. Assertions and documentation about optional properties (mentioned above) will really help.

[zepumph]

And I propose that we should "filter before propagating" for both config and options going forward.

I'll do this going forward.

@samreid had spoke earlier about trying to develop a consistent pattern/tool to help with this. I definitely think that would be good to discuss.

[samreid]

We decided we are happy with the plan described above, @samreid will attempt to summarize the consensus pattern in phet-info/docs/phet-software-design-patterns.md

[samreid]

@jessegreenberg can you please review the change above? If all is well, please close the issue.

[jessegreenberg]

Thanks @samreid. In #930 (comment) it was mentioned that assertions for should be added for required config properties. I added a statement about that, can you please re-check?

[samreid]

Thanks for reminding me about the asserts, I also changed it to check for null so that values of 0 wouldn't fail out, and I added an assert message. Can you double check?

[jessegreenberg]

Very nice, thanks @samreid. Closing.

[samreid]

I used this pattern in WaveInterferenceTimerNode, which has a required option. It seemed awkward to pass config through to options. I don't see a way out of that other than just using either options or config everything (and they can have arbitrary number of optional or required args).