Skip to content

Simplify Constants#1614

Merged
devemux86 merged 1 commit intomasterfrom
constants
Jan 6, 2025
Merged

Simplify Constants#1614
devemux86 merged 1 commit intomasterfrom
constants

Conversation

@devemux86
Copy link
Collaborator

No description provided.

@devemux86 devemux86 added this to the 0.23.0 milestone Jan 6, 2025
@devemux86 devemux86 merged commit 289ff42 into master Jan 6, 2025
@devemux86 devemux86 deleted the constants branch January 6, 2025 08:40
@Sublimis
Copy link
Contributor

Sublimis commented Jan 6, 2025

A vote against. Now, if a future programmer wants to test something related to hs color, they have to go through the code in various places, and probably miss something. This constant may or may not change, no one knows, but then again, the constant PI will never change for all eternity, yet using the number 3.14 directly is considered bad practice.

There is also the impossibility to javadoc such hard-coded constants, as well as other practical disadvantages.

@devemux86
Copy link
Collaborator Author

These are not global constants and are only used in one place.

If there is a need for multiple usage in a function, it is better to host them within the function's package.

@Sublimis
Copy link
Contributor

Sublimis commented Jan 6, 2025

If by "global" you mean system-global and not library-global, then there might never be any more Constants other than the max memory.

HS color default was used in 3 files from 3 packages.

Make it a constant inside RenderThemeHandler then? Maybe make it a Parameter? Not seeing any downside to making a default hs color a "semi-constant" parameter like the rest of the class.

Anything would be better than hard-coded values.

But you know, whatever. Just thinking about a poor future fellow programmer sifting the code for instances of 0xff000000, like we had to do for other constants on several occasions (so it's not just theoretical whining).

@devemux86
Copy link
Collaborator Author

Default colors are declared the same way in all theme rules, e.g. Caption.
The same happens for all default values of the xml schema in the code,
something that could be improved in the future for all theme rules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants