[java] Explain the existence of AvoidFinalLocalVariable in it's description#1482
Conversation
Generated by 🚫 Danger |
adangel
left a comment
There was a problem hiding this comment.
Thanks for the PR!
Would you mind, changing the description into multiple lines? Note: You do not need to use a CDATA here, just use multiple lines, so that I don't need to scroll horizontally 😄
I had another look at the rule - I can only guess, what the intention of it is: Instead of defining a constant as a final local variable, it should rather be defined as a static final field on the class level (that's how you usually define a constant in java). This of course assumes, that any final local variable initialized with a (constant) literal is actually a constant and has a useful meaning outside the scope of the method.
Maybe the rule was created to avoid duplicated definitions of the same constants in different methods, thus to foster reuse of a constant across methods?
The constant theory seems to be correct, e.g. https://sourceforge.net/p/pmd/bugs/790/ was arguing that final String foo = "foo"; should be defined as a private static final String FOO = "foo";.
Note: The rule is now 11 years old and initially flagged any final local variable without exceptions (14574cf#diff-0a0aaf0e7641329403dac0315535d3bdR415)
|
If the intent is really to extract a constant as a class variable, then the rule should be called ConstantValueMayBeAField, or AvoidDuplicatingLiterals, or something like that. The fact that the local var is final has nothing to do with it, so the name "AvoidFinalLocalVariable" is not adapted. I'd argue for this rule to be deprecated, and some other rule to take its place that does what the rule was intended for, with a relevant name. |
|
In fact we already have an AvoidDuplicateLiterals rule in category errorprone (which applies only to string literals). If a constant is not duplicated, then it can stay local to a method without problem: it's probably only relevant to the method, and assuming otherwise would be prone to false positives. Because of this I don't see a reason to keep AvoidFinalLocalVariable anymore. |
|
Refs #811 We should probably also mention, that there is a contradictory rule LocalVariableCouldBeFinal. Having both enabled won't work: One tells, to not use final, the other tells to use final... I'll try to summarize, what we need to do on master (for 6.11.0):
And then let's remove the rule in 7.0.0. |
Before submitting a PR, please check that:
master. The PMD team will merge back to support branches as needed../mvnw clean verifypasses. This will build and test PMD, execute PMD and checkstyle rules. Check this for more infoPR Description:
Rather than being a generally useful rule,
AvoidFinalLocalVariablecontradicts a good code style in most cases and only serves to avoid a special case (local literals) which it doesn't distinguish.