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

nonstandard pattern for static constants #281

Closed
pixelzoom opened this issue Dec 19, 2018 · 3 comments
Closed

nonstandard pattern for static constants #281

pixelzoom opened this issue Dec 19, 2018 · 3 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Dec 19, 2018

Related to code review #259.

This was originally a REVIEW comment, but I'm promoting it to an issue.

During a recent ES6 discussion (led by @jonathanolson, if I recall correctly) we discussed the lack of static constants in JS, and the alternatives. And I believe that we concluded that this pattern was the most straightforward:

class SomeClass {
  ...
}

SomeClass.SOME_STATIC_CONSTANT = 5;

return someNamespace.register( 'SomeClass', SomeClass );

@samreid is using a different pattern in Wave Interference. For example, in WaveScreensModel:

class WavesScreenModel {
...
  static get EVENT_RATE() {
    return EVENT_RATE;
  }
}

return waveInterference.register( 'WavesScreenModel', WavesScreenModel );

This pattern is also used for:
LightWaveGeneratorNode.DEFAULT_OPTIONS
WavesScreenModel.EVENT_RATE
WavesScreenView.SPACING

A couple of potential issues with this pattern:

(1) It obfuscates what is really happening - it's a function masquerading as a constant.
(2) It violates the naming convention for functions.

This might be something that we need to discuss at dev meeting, so labeling accordingly.

@jonathanolson
Copy link
Contributor

I have a few cases of that pattern (from before the decision was made) in fractions-common. I'm happy to refactor to the "new" style (assigning myself).

@samreid
Copy link
Member

samreid commented Dec 21, 2018

Proposed fix committed, @pixelzoom can you please review this change for Wave Interference? I'll create a separate issue for @jonathanolson for fractions common. If this issue looks good for wave-interference, it can be closed.

@pixelzoom
Copy link
Contributor Author

👍 Closing.

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