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 >