My test for whether enforce a guideline in a PR review is: Would I be willing
to automate enforcing it in CI?
I am -0 on this particular guideline.
IntelliJ offers two competing inspections for Java coding style:
* Local variable or parameter can be final
* Unnecessary 'final' on local variable or parameter
Both of these are (I think) disabled by default in IntelliJ. And neither
satisfies my “I’d be willing to automate enforcing it” test.
Dale
From: Mark Hanson <hans...@vmware.com>
Date: Monday, April 19, 2021 at 11:08 AM
To: dev@geode.apache.org <dev@geode.apache.org>
Subject: Re: [VOTE] Requiring final keyword on every parameter and local
variable
Hi Jake,
I agree with everyone's point about final being a useful, I just don't find it
useful enough to do anything manually across the code base at this point.
I believe first in foremost in no code warnings. Intellij warns about variables
that can be final, so I make them final as it finds them. It is very rare that
I am writing new code at this point. I have spent the last year bug fixing
other people's code. From my standpoint, original intent is largely moot. So,
for me, the question is do I go through code that is not mine and mark it all
as final (where appropriate)? Sure, in code that I touch. In code the rest of
the codebase, I think there are bigger fish to fry before we get to final.
It seems like the larger portion of the consensus is to recommend that
variables are marked final (where appropriate) as we find them or create them.
That seems like the going forward approach consensus.
I will be blunt though. This seems nitpicky *compared* to the number of code
warnings there are and the fact that people are not actively fixing all of the
warnings.
If we don't come to the same consensus about all warnings this seems like
painting a rusted car, sure it makes it all look the same, but does very little
for the underlying problems. Now to undermine my own argument a little, I think
that especially in release blockers, we want to touch as little code as
possible, so I am flexible there.
I would also like to agree with Udo about final really not being very good
compared to Mutable and Immutable objects in other languages.
Thanks,
Mark
On 4/15/21, 7:49 PM, "Udo Kohlmeyer" <u...@vmware.com> wrote:
@jake, you are correct, I did miss the LOCAL variable out of the subject.
:face_palm:
Yes, adding "final" to LOCAL variables will be HUGELY beneficial in this
regard. Have we seen any performance improvement after having refactored this
method? Have you been able to measure any improvements?
--Udo
On 4/15/21, 1:19 PM, "Jacob Barrett" <jabarr...@vmware.com> wrote:
> On Apr 14, 2021, at 7:46 PM, Udo Kohlmeyer <u...@vmware.com> wrote:
> @Jake the idea of smaller methods is great and we should ALWAYS
strive for that. But that argument is completely irrelevant in this discussion.
As making method arguments final does not naturally guide a developer to
creating smaller methods. Nor does a smaller method mean it can/will be jitted.
Too many factors (to discuss here) are part of that decision, also it is not
relevant in this discussion. But more on that topic read THIS.
The original subject is in regards to parameters and local variables.
Irrelevant is certainly an opinion you are welcome to have but let me
challenge you. Goto DistributedCacheOperation._distribute().
First challenge, look at around line 333:
boolean reliableOp = isOperationReliable() &&
region.requiresReliabilityCheck();
Without scrolling do you see that variable used? Nope, because it is
first used on line 439, ~100 lines away. Does it mutate between there, well I
can search for all uses and find out or I could be nice to the next person and
intend for it to never mutate by adding final. Intent communicated!
Second challenge, mark all the local variables in the method as final.
Now make it compile without introducing more mutable variables. At the end of
this journey you will have about a dozen unit testable methods and a
_distribute method that goes from ~370 lines to ~90 with no mutable local
variables.
I argue it is relevant as good guardrail for writing good code. While
we should ALWAYS strive for it we don’t. Every little nudge helps.
-Jake
