Use Enumerations instead of String Unions

[marlitas]

From: #109
I'm curious about the decision to use string unions instead of enumerations for various states including:

• CoinStates
• ExperimentMeasurementState
• QuantumCoinStates
• SystemType
• SpinMeasurementState

For any of these that are then tracked by Properties it feels like we're missing out on valid values support, and I also believe that an enumeration provides more robust support in some of the accessibility scenarios you're finding yourself in ( see CoinState).

Might be good to take a second pass and consider refactoring these to be enums.

[AgustinVallejo]

Nice question @marlitas. I agree with you and vouch for using Enumeration as much as possible. However, it boils down to a discussion of robustness vs simplicity/readability. John and I had that discussion multiple times and brought it up at dev meeting. Here's the summary:

So in short, it depends... That said, I still think Enumeration prepares you better for future additions to the code, like a11y, as you mention yourself, and frees us from having to invent new patterns to accommodate string unions. As seen here #92 (comment)

Back to you.

[marlitas]

As seen here #92 (comment)

I would go one step further and make that DerivedProperty a DynamicProperty. You won't have to map the value out of it when you pass it into a PatternStringProperty and I think it's also easier to read... but I digress.

Running into this also in CoinsModel:

    this.experimentTypeProperty = new Property<SystemType>( 'classical', {
      tandem: providedOptions.tandem.createTandem( 'experimentTypeProperty' ),
      phetioValueType: StringUnionIO( SystemTypeValues ),
      validValues: SystemTypeValues,
      phetioFeatured: true
    } );

If SystemType were an enumeration you wouldn't have to define the phetioValueType nor would you have to pass in validValues... maybe its not needed in all scenarios, but I'm leaning towards some of these (especially if they are used in a Property) being converted to Enumerations. In the end its your and @jbphet's decision though.

[marlitas]

Running into this again in CoinExperimentButtonSet.ts I believe you could probably simplify this DerivedProperty by storing the relevant strings with the EnumValue:

  // Create the text for the button whose label changes based on the state.
    const revealHideButtonTextProperty = new DerivedStringProperty(
      [
        QuantumMeasurementStrings.hideStringProperty,
        QuantumMeasurementStrings.revealStringProperty,
        QuantumMeasurementStrings.observeStringProperty,
        coinSet.measurementStateProperty
      ],
      ( hideString, revealString, observeString, experimentState ) => {
        let labelString;
        if ( experimentState === 'revealed' ) {
          labelString = hideString;
        }
        else if ( coinSet.coinType === 'classical' ) {
          labelString = revealString;
        }
        else {
          labelString = observeString;
        }
        return labelString;
      }
    );

[AgustinVallejo]

I also vote for using EnumerationValue. Up to @jbphet for consideration if it's worth the refactor.

[marlitas]

PhotonsScreenView.ts has another spot where using EnumerationValue and storing the related stringProperty in that value would simplify things.

const EXPERIMENT_MODE_TO_STRING_MAP = new Map<ExperimentModeType, LocalizedStringProperty>(
  [
    [ 'singlePhoton', QuantumMeasurementStrings.singlePhotonStringProperty ],
    [ 'manyPhotons', QuantumMeasurementStrings.manyPhotonsStringProperty ]
  ]
);

This trickles down into SceneSelectorRadioButtonGroup where you have to have this weird assertion now instead of just relying on the passed in Property:

assert && assert( valueToStringMap.size === 2, 'SceneSelectorRadioButtonGroup requires exactly two items' );

[marlitas]

Another spot in PhotonTestingArea.ts where stringProperties stored in EnumerationValues would simplify code:

const particleBehaviorModeRadioButtonGroup = new AquaRadioButtonGroup<SystemType>(
      model.particleBehaviorModeProperty as PhetioProperty<SystemType>,
      SystemTypeValues.map( behaviorMode => {

          const nameProperty = behaviorMode === 'classical' ?
                               QuantumMeasurementStrings.classicalStringProperty :
                               QuantumMeasurementStrings.quantumStringProperty;

          const tandemRootName = behaviorMode === 'classical' ? 'classical' : 'quantum';
          const tandemName = `${tandemRootName}RadioButton`;

          return {
            value: behaviorMode,
            createNode: () => new Text(
              nameProperty,
              { font: new PhetFont( 15 ), maxWidth: 100 }
            ),...

[AgustinVallejo]

Adding to this, just saw in MOTHA the use of StringUnionProperty.ts: This type automatically specifies
validValues and the phetioType for convenience. So it could be a good first step to at least use this class

[marlitas]

Another scenario where EnumerationValue might help. You can tie the color to the enumeration value and use a DynamicProperty instead of creating a DerivedProperty. This one isn't as convoluted to read so I'm not as worried about it, just showing as an example:

  const experimentControlButtonColorProperty = new DerivedProperty(
      [ model.measurementStateProperty ],
      measurementState => measurementState === 'observed' ?
                          QuantumMeasurementColors.experimentButtonColorProperty.value :
                          QuantumMeasurementColors.startMeasurementButtonColorProperty.value
    );

[AgustinVallejo]

I mean, after starting this refactor, it seems to me the benefit of EnumerationValue is not as worth the change. While some of the examples listed here definitely look more clean, there's a number of new lines added elsewhere for each one. Last commit even added more code than it removed. So at this stage of development I would close this, and rather keep it in mind for future implementations.

Nonetheless, the only example left unaddressed is #120 (comment), as it combines different enumerations so its not as simple as storing the label.

[AgustinVallejo]

@jbphet suggested we tried StringUnionProperty, as they do in MOTHA. I tried it on the remaining string unions and it only kinda helped presetPolarizationDirectionProperty by letting us get rid of the phetioValueType option. The other string unions that resisted the change via EnumerationValue and now via StringUnionProperty are CoinStates, since they are not only string union but a composition of other possible values, the type logic needed to make it work starts getting more complicated, especially out of the declarations, i.e. in other type castings put in place to make those work. I bet it's possible, but then again, not a refactor I think we should be spending much time on... So I'll even ditch the StringUnionProperty change to not have now three different ways of doing this enumeration pattern.

Back to JB for final remarks on this. Also feel free to review the above commits.

[jbphet]

I spent some time reviewing the issue and the changes that @AgustinVallejo has made, and I think we're good to close this. We now have several enumerations that are "rich" in the sense that they encapsulate other information such as strings and colors, and others that are simple and have just a value. This, IMO, matches the spirit of the consensus referenced in #120 (comment). I'll also make sure to keep these tradeoffs in mind for the next sim I work on.