Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify PDLPreferences.ts #199

Closed
pixelzoom opened this issue Feb 27, 2024 · 1 comment
Closed

Simplify PDLPreferences.ts #199

pixelzoom opened this issue Feb 27, 2024 · 1 comment
Assignees

Comments

@pixelzoom
Copy link
Contributor

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
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>( 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<BinStrategy>( PDLQueryParameters.binStrategy as BinStrategy, {
+    validValues: BinStrategyValues,
+    tandem: Tandem.PREFERENCES.createTandem( 'binStrategyProperty' ),
+    phetioFeatured: true
+  } )
 };
 
 projectileDataLab.register( 'PDLPreferences', PDLPreferences );
@samreid
Copy link
Member

samreid commented Feb 27, 2024

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

@samreid samreid closed this as completed Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants