Some options extend but apply the same value

[samreid]

Discovered during #14

I noted this code:

  class ExploreEquationAccordionBox extends GQEquationAccordionBox {

    /**
     * @param {ExploreModel} model
     * @param {Object} [options]
     */
    constructor( model, options ) {
                                              
      options = _.extend( {
        tandem: Tandem.required
      }, options );

And also this code:

  class GQEquationAccordionBox extends AccordionBox {

    /**
     * @param {ExploreModel} model
     * @param {Node} content
     * @param {Object} [options]
     * @abstract
     */
    constructor( model, content, options ) {

      options = _.extend( {
        tandem: Tandem.required,

Since the tandem is required in the base class, it is redundant to specify its default as required in the subclass. When is it appropriate to have subclasses re-specify the same default value for an option? Only in cases where the base class value is likely to change.

I haven't checked if this pattern appears elsewhere, I thought I would bring up this case as one occurrence to be addressed, and @pixelzoom can resolve any other cases that are using this same pattern.

[pixelzoom]

tandem: Tandem.required (as I understand it) serves to indicate that instances of a class must be instrumented. I see no harm whatsoever in duplicating this, it's a non-issue. Efforts would be better focused on identifying common code that should not be specifying tandem: Tandem.required (which I believe is most common code).

Closing.

[samreid]

I'm trying to understand the scope of your argument with a thought experiment. For instance, let's say I had a base class with a default of stroke: 'blue'. Then I put a default of stroke:'blue' in the subclass. Then the argument would be: "stroke: 'blue' serves to indicate that instances of the class have a blue stroke. I see no harm whatsoever in duplicating this, it's a non-issue.". Is this a reasonable comparison? If not, why would the argument apply for tandem but not for stroke? Shouldn't the subclass be aware of its parents' defaults? I'm happy if we close this issue without taking action, but I would like to better understand your viewpoint.

[pixelzoom]

tandem: Tandem.required is required in both superclass (GQEquationAccordionBox) and subclass (ExploreEquationAccordionBox ) because options.tandem.createTandem is called in both the superclass and subclass. And the base class AccordionBox has no default value for tandem option. So no change required here.

Is this a reasonable comparison?

Yes. But not worth the effort involved in hunting down redundant specification of stroke: 'blue' or tandem: Tandem.required.

Shouldn't the subclass be aware of its parents' defaults?

Reasons why a subclass may redundantly specify an option value:

• Sometimes it's easier and less costly to just specify the value, rather than searching through a class/mixin hierarchy to identify the default. There are numerous examples in scenery.
• Defensive programming when you think the default in a superclass may (or should) change. PhET has plenty of these, common-code defaults that are not generally useful, they are the values used by whatever sim first used/created the component. Using (and duplicating) light blue for sun buttons is a prime example (see RectangularButtonView and RoundButtonView).
• Your code underwent a refactor and you missed a couple of option values that now match a superclass. Should you spend the time to exhaustively locate these? It probably depends on the option, but my opinion is decidedly "no" for tandem: Tandem.required.
• The option serves some bug-identification function and is easier (and less costly) to mindlessly add everywhere. That's probably what happened here with tandem: Tandem.required.

[samreid]

And the base class AccordionBox has no default value for tandem option.

Please see
https://github.com/phetsims/sun/blob/11e15ecb24967fd010b93c3ffacef01eb989d629/js/AccordionBox.js#L45-L46

  function AccordionBox( contentNode, options ) {

    var self = this;

    options = _.extend( {

      // {Tandem}
      tandem: Tandem.required,

tandem: Tandem.required is required in both superclass (GQEquationAccordionBox) and subclass (ExploreEquationAccordionBox ) because options.tandem.createTandem is called in both the superclass and subclass.

I see, that makes sense.

Reasons why a subclass may redundantly specify an option value:

These sound reasonable, should we add them to the phet-info/doc/phet-software-design-patterns.md or some other document? I'm not sure everyone on the team is aware of those points.

[pixelzoom]

Mea culpa - I looked for the tandem default where it's supposed to be. AccordionBox violates the "group phet-io options at the end of defaults" convention. tandem and other phet-io options are separated by 50 lines. Should this be fixed? (This btw is another good example of why it's expensive to comb through superclasses looking for default values.)

  function AccordionBox( contentNode, options ) {

    var self = this;

    options = _.extend( {

      // {Tandem}
46      tandem: Tandem.required,

...

96      // phet-io support
97      phetioType: AccordionBoxIO,
98      phetioEventType: 'user',

      // a11y options
      tagName: 'div',

      // Options will be provided to both title bar nodes
      titleBarOptions: null
    }, options );

These sound reasonable, should we add them to the phet-info/doc/phet-software-design-patterns.md or some other document? I'm not sure everyone on the team is aware of those points.

These seem like common sense to me. But if you think they would be useful in a document, feel free to add them.