Kirk speaks my mind. -1 on this requirement
On 4/21/21, 11:29 AM, "Kirk Lund" <kl...@apache.org> wrote:
-1 as I've already explained in my DISCUSS thread for this topic, I don't
think final gives sufficient benefit for local variables or method
parameters because it only prevents reassigning object pointers and
primitive values.
I favor individuals making a personal decision on whether or not they want
to use 'final' in this way without requiring others to do the same (and NOT
requiring this use of 'final' in PR reviews, although "suggesting" is ok).
Note that JDK and all other libraries I looked through do not use final in
this way.
*I'm willing to change my vote to -0 if*:
1. we qualify the requirement to only apply to method and constructor
parameters (not local vars)
2. we find a way to introduce automation for enforcement (such as we do
with RAT for license headers)
*I'm willing to change my vote to +1 if *we decide to require 'final'
parameters (and even local vars) as part of a bigger shift towards
improving code quality by embracing Clean Code (ala Bob Martin):
1. we commit to developing a robust and thorough coding standard that
includes requiring true unit tests and passing in dependencies via class
constructors or methods
2. we revisit our DOD and enforce it in PR reviews
I recommend that contributors use IntelliJ inspections (or comparable
guideline for anyone who doesn't want to use IntelliJ) that will assist us
in avoiding and fixing long methods when writing new methods and when
editing/refactoring existing methods (see below)
Method metrics (see IntelliJ > Preferences > Editor > Inspections):
- Method with too many parameters
- Overly complex method
- Overly long method
- Overly nested method
The inspections can be customized for Geode as long as the values aren't
increased to the point of it being useless. For example, we would have to
agree as a community what the "maximum number of parameters" is for the
"Method with too many parameters" inspection.
We could even export an inspections profile to be stored in the Geode repo
and used by contributors.
-Kirk
On Wed, Apr 14, 2021 at 12:55 PM Kirk Lund <kl...@apache.org> wrote:
> Our coding standard and our Design Decisions does NOT require using final
> on parameters and local variables.
>
> Please do NOT request that I add or keep final on parameters or local
> variables unless the community votes and decides that every parameter and
> local variable should require the final keyword. I coded this way in the
> past and I found that it resulted in noisy code with no benefit. We can
> argue about using this keyword all you want but the fact is I'm against
it,
> and I will not embrace it unless this community decides that we need to
use
> it.
>
> Using final on instance or class fields does have a concurrency benefit
> and I support that only.
>
> If you want to add final to every single parameter and local var in the
> Geode codebase, then now is your chance to vote on it. Please vote.
>
> Thanks,
> Kirk
>
>
