Any interest in adopting https://projectlombok.org in Geode? Lombok allow a
more readable "val" vs "var" syntax instead of "final" vs "" which could make
it easier to do the right thing without "increasing visual noise".
On 4/19/21, 12:40 PM, "Dale Emery" <dem...@vmware.com> wrote:
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
