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

Unfamiliar TS pattern in CountingGameLevel.ts #96

Closed
pixelzoom opened this issue Dec 15, 2021 · 7 comments
Closed

Unfamiliar TS pattern in CountingGameLevel.ts #96

pixelzoom opened this issue Dec 15, 2021 · 7 comments

Comments

@pixelzoom
Copy link
Contributor

For #84

Please explain and document why these (duplicate?) underscore fields are needed in CountingGameLevel.ts:

  private readonly _playObjectTypeProperty: EnumerationProperty;
  public readonly playObjectTypeProperty: IReadOnlyProperty<Enumeration>;
  private readonly _isObjectsRepresentationProperty: BooleanProperty;
  public readonly isObjectsRepresentationProperty: IReadOnlyProperty<boolean>;
@Luisav1
Copy link
Contributor

Luisav1 commented Dec 15, 2021

@chrisklus Wanted to try out a pattern that @samreid suggested. The purpose of duplicating it is to have one that is read/write privately in the class and one that is read only publicly. While there is duplicated attributes, there is still only one Property.

@pixelzoom pixelzoom removed their assignment Dec 15, 2021
@pixelzoom
Copy link
Contributor Author

That would be good to document in the code, since it's not at all obvious what's going on.

And when you're doing that, see also #97, so that the documentation is with the declaration, not the instantiation.

@pixelzoom
Copy link
Contributor Author

It looks like this pattern is not limited to CountingGameLevel.ts. I see it in other places like Subitizer.js. So another (alternative? additional?) place that you might consider describing it is in implementation-notes.js, which often has a Patterns section.

Luisav1 added a commit that referenced this issue Dec 16, 2021
… Properties in CountingGameLevel.ts and Subitizer.ts. See #96.
@Luisav1
Copy link
Contributor

Luisav1 commented Dec 16, 2021

Fixed in the commit above. Back to @chrisklus to review and close if all is good.

@Luisav1 Luisav1 assigned chrisklus and unassigned Luisav1 Dec 16, 2021
chrisklus added a commit that referenced this issue Feb 15, 2022
…ead only Properties in CountingGameLevel.ts and Subitizer.ts. See #96."

This reverts commit 25522c3.
@chrisklus
Copy link
Contributor

While I really like the type safety of this pattern, it's not something that I've started using elsewhere in NumberPlay or other sims. I think that especially for these relatively simple cases, it's not worth the name duplication/verbosity of having a private and public Property. Perhaps Property.js will have better support to do this in the future without needing to duplicate. For now, I'm removing it to unify Game screen code with the rest of the sim.

@Luisav1 thank you for doing this write up! I'll bring it back from the git history if someday it seems worth it to outfit this sim with private read/write and public readonly Properties. For now, I reverted the commit in the implementation notes. Closing.

@zepumph
Copy link
Member

zepumph commented Mar 23, 2022

I brought this up in the March 3 agenda of Developer meeting, and I thought that this was the preferred strategy when you have a need to change something internally but to make it readonly publicly. I just wanted to post here noting that I will be using this in Ratio and Proportion, and that it is documented in the typescript convention document: https://github.com/phetsims/phet-info/blob/635d23558371e8b8b60bffb4bfd998d4503afb12/doc/typescript-conventions.md#access-modifiers

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 23, 2022

... it is documented in the typescript convention document:

An example would be nice to include with the description.

zepumph added a commit to phetsims/phet-info that referenced this issue Mar 24, 2022
samreid pushed a commit to phetsims/phet-info that referenced this issue Apr 23, 2022
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

4 participants