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
>
