search

phetsims / projectile-data-lab

"Projectile Data Lab" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 0 forks source link

Simplify PDLPreferences.ts #199

Closed pixelzoom closed 5 months ago

pixelzoom commented 5 months ago

For code review #32 ...

I find PDLPreferences.ts a bit difficult to read. I suspect that it might have just evolved to this state, because I can't see any reason for intentionally structuring it this way. The constants don't seem to be necessary, and the entire file could be simplified as in the patch below.

patch ```diff Subject: [PATCH] add temporary logging for position of draggable objects, https://github.com/phetsims/faradays-electromagnetic-lab/issues/64 --- Index: js/common/PDLPreferences.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/PDLPreferences.ts b/js/common/PDLPreferences.ts --- a/js/common/PDLPreferences.ts (revision b35c3deceaf1873330fb0e08b4af9e9cfafc705a) +++ b/js/common/PDLPreferences.ts (date 1709062480349) @@ -14,33 +14,28 @@ import PDLQueryParameters from './PDLQueryParameters.js'; import projectileDataLab from '../projectileDataLab.js'; -// phet-types.d.ts does not support inferring string union types for string query parameters, so we need to cast -const binStrategy = PDLQueryParameters.binStrategy as BinStrategy; +const PDLPreferences = { -// the top checkboxes are left aligned with the play area checkboxes, so their max width is smaller to accommodate -// for the accordion box margin -const AUTO_GENERATE_DATA_PROPERTY = new BooleanProperty( PDLQueryParameters.autoGenerateData, { - tandem: Tandem.PREFERENCES.createTandem( 'autoGenerateDataProperty' ), - phetioFeatured: true, - phetioDocumentation: 'When true, the data is instantly and automatically generated when the launch button is pressed.' -} ); + // the top checkboxes are left aligned with the play area checkboxes, so their max width is smaller to accommodate + // for the accordion box margin + autoGenerateDataProperty: new BooleanProperty( PDLQueryParameters.autoGenerateData, { + tandem: Tandem.PREFERENCES.createTandem( 'autoGenerateDataProperty' ), + phetioFeatured: true, + phetioDocumentation: 'When true, the data is instantly and automatically generated when the launch button is pressed.' + } ), -const PROJECTILE_TYPE_AFFECTS_SPEED_PROPERTY = new BooleanProperty( PDLQueryParameters.projectileTypeAffectsSpeed, { - tandem: Tandem.PREFERENCES.createTandem( 'projectileTypeAffectsSpeedProperty' ), - phetioFeatured: true, - phetioDocumentation: 'When true, the projectile type affects its mean launch speed.' -} ); + projectileTypeAffectsSpeedProperty: new BooleanProperty( PDLQueryParameters.projectileTypeAffectsSpeed, { + tandem: Tandem.PREFERENCES.createTandem( 'projectileTypeAffectsSpeedProperty' ), + phetioFeatured: true, + phetioDocumentation: 'When true, the projectile type affects its mean launch speed.' + } ), -const BIN_STRATEGY_PROPERTY = new StringUnionProperty( binStrategy, { - validValues: BinStrategyValues, - tandem: Tandem.PREFERENCES.createTandem( 'binStrategyProperty' ), - phetioFeatured: true -} ); - -const PDLPreferences = { - autoGenerateDataProperty: AUTO_GENERATE_DATA_PROPERTY, - projectileTypeAffectsSpeedProperty: PROJECTILE_TYPE_AFFECTS_SPEED_PROPERTY, - binStrategyProperty: BIN_STRATEGY_PROPERTY + // phet-types.d.ts does not support inferring string union types for string query parameters, so we need to cast + binStrategyProperty: new StringUnionProperty( PDLQueryParameters.binStrategy as BinStrategy, { + validValues: BinStrategyValues, + tandem: Tandem.PREFERENCES.createTandem( 'binStrategyProperty' ), + phetioFeatured: true + } ) }; projectileDataLab.register( 'PDLPreferences', PDLPreferences ); ```
samreid commented 5 months ago

Good idea! I applied the patch, and updated documentation. It seems good, so I will close the issue.