-
Notifications
You must be signed in to change notification settings - Fork 5
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
Anti-pattern for defining static readonly class properties. #278
Comments
This problem is widespread. For example, looking only at IOType definitions: @samreid I see this anti-pattern throughout cck-common:
@jonathanolson in density-buoyancy-common and gravity-and-orbits:
@jbphet in greenhouse-effect, you're instantiating these inside the class definition, but missing
@marlitas in mean-share-and-balance:
@jonathanolson in scenery:
|
Avoiding this pattern for most of the above cases however seems quite helpful. |
Addressed the MSaB occurrence. This seems useful and writing a lint rule is a good idea. |
@marlitas Static constants should not be assigned in the constructor, because the constructor is called every time that |
I agree these are antipatterns but also wanted to point out the overlap in #275 in which we may (as a long term solution) combine IOTypes with the core types. |
Yes, there's indeed overlap with #275. But I want to emphazied that, while the examples I've pointed to involve IOTypes, the antipattern applies to all statics, not just IOTypes. Do a search for "public static" and you'll find many examples. |
Thanks for pointing this out @pixelzoom. I made changes in quite a few places in greenhouse-effect, beyond the list shown above. @marlitas - I agree that a lint rule for this would be helpful. |
Here is the example AST, it should be easy to make a lint rule to flag when Might also be easy to add an auto-fix. EDIT: Well... easy to find these in the AST but not easy to determine the intent, lots of |
Dev meeting Nov 17 2022. We agreed that making public static class properties @zepumph: I see 262 usages where this pattern is happening. Maybe half of them can be fixed. So that is maybe 2 hours of work to do it all. @pixelzoom & @AgustinVallejo: Great! Here is a regex from @zepumph that may help |
@AgustinVallejo and I met about this after today's dev meeting. He'll use the regex |
In the above commits, I addressed a bunch more cases. There are still some cases that have not been addressed (as indicated in #278 (comment)) because they are problematic. And there are some cases in sun and scenery that are just a little too much work to untangle and test. But I don't feel like we need to address every case for this issue (diminishing returns...), and I feel like we're at a good stopping point. @AgustinVallejo do you agree? If so, feel free to close this issue. If not, what else should we do? |
I'm seeing a pattern of definiting static class properties that is concerning. It results in those constants being writeable when they should be
readonly
.For example, here's Vector2.ts:
All of the
static
properties should bestatic readonly
. And they should be instantiated inside theclass
definition, not outside the class definition. Like this:A couple of things for discussion at developer meeting:
(1) PSA and recommendation not to use this pattern. Statics that are read-only should have the
readonly
modifier.(2) Can we write a lint rule to prevent this, and identify all of the existing cases?
The text was updated successfully, but these errors were encountered: