[DISCUSS] Requiring final keyword on every parameter and local variable

[Kirk Lund <kl...@apache.org>]

Moving the discussion to this thread. Sorry for starting off with a VOTE.

I'm primarily interested in Maintainability
https://en.wikipedia.org/wiki/Software_quality#Maintainability, and my
concern regarding using "final" on method parameters or local variables is
about Code Readability: understanding what the code does at a glance and
being able to spot bugs and mistakes more easily.

Infrequently used keywords such as "final" on method parameters and local
variables cause "noise" for me such that it slows down my understanding of
what the code does. I find that I have to spend more time studying it and
it takes longer than a "glance."

Now, if the keyword buys us enough to justify the "noise" then it's worth
it (such as for long methods), but I am arguing that small methods of a
dozen or less lines do NOT benefit from having "final" on any of its
variables or parameters.

I also really, really, really want to encourage this community to refactor
the code. In other words, if you touch a long method, PLEASE refactor it to
break it up into smaller, well-named methods with descriptive parameters
and local variables that are only ever assigned once. Then each method is
very easy to understand at a glance and we no longer need "final" on any of
its vars or params to indicate that it is never reassigned... because we
know at a glance that it's only assigned once.

Developers also have a tendency to look at code and then emulate it. If
they're looking at phenomenally long methods that aren't unit tested and
have short non-descriptive var names, then they're going to repeat that
both when maintaining such code and when writing new code. We don't want
that, or at least I don't want that.

Consistency (such as always using "final" on all method parameters) results
in the developer getting used to it and expecting it so that you recover
some of that speed in understanding what you're studying without stumbling
over "noise" or unnecessary syntax and structures.

I believe that simplicity and understandability is more important than
having the "final" keyword on just one out of five parameters on one
method. As I refactor the code, I make sure that the methods end up short
and succinct so that "final" is no longer needed on that one parameter.

Introducing a coding standard would help us immensely. The standard can
include important things like:
* Use descriptive variable and method names that are spelled out.
* Keep the methods short.

There are tons of best practices, corrections for code smells, and
conventions for highly readable and maintainable code that we could and
should be adopting.

The bottom line is I don't really care about "final" -- using it or NOT
using it -- what I care about is code quality and making the code readable
and maintainable while following a well-thought out and group-written
coding standard.

-Kirk

Re: [DISCUSS] Requiring final keyword on every parameter and local variable

[Kirk Lund <kl...@apache.org>]

PS: Here is my version of that constructor after refactoring the class:

PartitionedRegionClearMessage(Set<InternalDistributedMember> recipients,
PartitionedRegion region,
    ReplyProcessor21 processor, PartitionedRegionClearMessage.OperationType
operationType,
    RegionEventImpl event) {
  super(recipients, region.getPRId(), processor);
  partitionedRegion = region;
  this.operationType = operationType;
  callbackArgument = event.getRawCallbackArgument();
  eventId = event.getEventId();
}

And I could rename "region" to "partitionedRegion", etc.

On Thu, Apr 15, 2021 at 12:00 PM Kirk Lund <kl...@apache.org> wrote:

> Here's a quote about code reviews in Clean Code in Python:
>
> We should invest time in code reviews, thinking about what is good code,
>> and how readable and understandable it is. When looking at the code written
>> by a peer, you should ask such questions as:
>> * Is this code easy to understand and follow to a fellow programmer?
>> * Does it speak in terms of the domain of the problem?
>> * Would a new person joining the team be able to understand it, and work
>> with it effectively?
>
>
> And I would add in:
> * Does the code follow community adopted coding standards?
> * Is the code complete from a DOD (Definition Of Done) perspective?
>
> Asking the contributor to add back in a "final" keyword on one parameter
> of a refactored method or constructor that is now small and easy to
> understand at a glance is a useful review in my opinion.
>
> If using "final" keyword on all method/constructor parameters and/or local
> variables becomes part of our coding standard, then it's a valid review
> because it's required and missing.
>
> If using "final" is NOT required and I remove "final" from the regionEvent
> parameter of the following constructor of a new class...
>
> PartitionedRegionClearMessage(Set<InternalDistributedMember> recipients,
> PartitionedRegion region,
>     ReplyProcessor21 processor,
> PartitionedRegionClearMessage.OperationType operationType,
>     final RegionEventImpl event) {
>   super(recipients, region.getPRId(), processor);
>   partitionedRegion = region;
>   op = operationType;
>   cbArg = event.getRawCallbackArgument();
>   eventID = event.getEventId();
> }
>
> ...then I'm saying it's not helpful and not desirable for a review to tell
> me to add that "final" keyword back in with a "Request Changes" on the PR.
> I also do NOT want add "final" keyword to all of the other parameters for
> consistency because it just adds a bunch of words without adding any value.
> This kind of review offends me, because I am striving for high quality
> clean code with short methods and constructors for the highest readability
> and maintainability that I can achieve.
>
> -Kirk
>
>>

Re: [DISCUSS] Requiring final keyword on every parameter and local variable

[Kirk Lund <kl...@apache.org>]

Here's a quote about code reviews in Clean Code in Python:

We should invest time in code reviews, thinking about what is good code,
> and how readable and understandable it is. When looking at the code written
> by a peer, you should ask such questions as:
> * Is this code easy to understand and follow to a fellow programmer?
> * Does it speak in terms of the domain of the problem?
> * Would a new person joining the team be able to understand it, and work
> with it effectively?


And I would add in:
* Does the code follow community adopted coding standards?
* Is the code complete from a DOD (Definition Of Done) perspective?

Asking the contributor to add back in a "final" keyword on one parameter of
a refactored method or constructor that is now small and easy to understand
at a glance is a useful review in my opinion.

If using "final" keyword on all method/constructor parameters and/or local
variables becomes part of our coding standard, then it's a valid review
because it's required and missing.

If using "final" is NOT required and I remove "final" from the regionEvent
parameter of the following constructor of a new class...

PartitionedRegionClearMessage(Set<InternalDistributedMember> recipients,
PartitionedRegion region,
    ReplyProcessor21 processor, PartitionedRegionClearMessage.OperationType
operationType,
    final RegionEventImpl event) {
  super(recipients, region.getPRId(), processor);
  partitionedRegion = region;
  op = operationType;
  cbArg = event.getRawCallbackArgument();
  eventID = event.getEventId();
}

...then I'm saying it's not helpful and not desirable for a review to tell
me to add that "final" keyword back in with a "Request Changes" on the PR.
I also do NOT want add "final" keyword to all of the other parameters for
consistency because it just adds a bunch of words without adding any value.
This kind of review offends me, because I am striving for high quality
clean code with short methods and constructors for the highest readability
and maintainability that I can achieve.

-Kirk

>