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 <[email protected]> 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
>
>>
