Re: [VOTE] Requiring final keyword on every parameter and local variable
I agree with Udo. Also, this shouldn't be a VOTE without discussion and I don't see a DISCUSS thread. On 4/14/21, 7:46 PM, "Udo Kohlmeyer" wrote: "+1" to ENCOURAGING developers to make "final" a requirement for method arguments. "-1" to making it a hard rule. If we want to enforce this rule on the basis of readability or performance, I fear that we might be beating the wrong horse here for the wrong reasons!! Short reasoning: If we really want to effect real lasting change, is to adopt principles that Rust (object owners) and Kotlin (immutable collections) make core to the language. Immutable POJOs and possibly an extension for collections to make intent of immutability clear would be far more effective. As code reviewers would be able to look at a method definition and instantly know what arguments are mutable. Adding "final" on fields, methods and classes has a far greater effect than that of method arguments. If we intent to enforce it based on readability, then we should really consider applying different approaches (as stated in the paragraph above) that can be used with far greater effectiveness. Longer reasoning: I think all arguments relating to "indicating authors intent" or "variable does not change" are great, but in reality we as developers should always assume that this was the intent. I think if we really want to improve our code base, let's opt for IMMUTABLE data objects. The "final" keyword has no benefit if you pass around POJOs (including Collections). As long as you don't change the instance reference, the "final" keyword has absolutely no effect. It does not mean that you cannot change every field internal of the POJO. The "final" keyword is only effective against the re-assignment of that variable/argument within a method. But a better practice is to avoid the re-assignment of any variable/argument in the first place. Kotlin has a great language (one of many) that method/function arguments are naturally final and this behavior cannot be changed. In addition there is a the feature that the collections are immutable and as a developer you have to be explicit if you want a collection to be mutable, by defining the collection as a "Mutable*" i.e MutableMap, MutableSet, MutableList @Jake the idea of smaller methods is great and we should ALWAYS strive for that. But that argument is completely irrelevant in this discussion. As making method arguments final does not naturally guide a developer to creating smaller methods. Nor does a smaller method mean it can/will be jitted. Too many factors (to discuss here) are part of that decision, also it is not relevant in this discussion. But more on that topic read THIS. --Udo On 4/15/21, 9:29 AM, "Jacob Barrett" wrote: If a method is longer than a handful of lines and I go in to refactor it I am going to start by making every variable I find final. Then I am going to figure out how to keep them final. By doing so you naturally produce smaller functional methods that are usually independently unit testable. Smaller methods can get jitted. Smaller methods can take less time to reach a safe point (see time to safe point issues in Java). For this reason I strongly prefer on final everywhere and if you think you need a variable to not be final you should use that as an indicator to refractor your code. So, +1 for me for final everywhere. Also, please stop declaring variables miles away from their use. > On Apr 14, 2021, at 12:55 PM, Kirk Lund 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
Re: [VOTE] Requiring final keyword on every parameter and local variable
Sorry Bruce, that's my lack of Apache finesse. I know you might expect me to remember it but I don't start discussions or votes often enough to remember. I'll be sure to read up on this process before starting any threads in the future. On Thu, Apr 15, 2021 at 7:32 AM Bruce Schuchardt wrote: > I agree with Udo. Also, this shouldn't be a VOTE without discussion and I > don't see a DISCUSS thread. > > On 4/14/21, 7:46 PM, "Udo Kohlmeyer" wrote: > > "+1" to ENCOURAGING developers to make "final" a requirement for > method arguments. > "-1" to making it a hard rule. > > If we want to enforce this rule on the basis of readability or > performance, I fear that we might be beating the wrong horse here for the > wrong reasons!! > > Short reasoning: > > If we really want to effect real lasting change, is to adopt > principles that Rust (object owners) and Kotlin (immutable collections) > make core to the language. Immutable POJOs and possibly an extension for > collections to make intent of immutability clear would be far more > effective. As code reviewers would be able to look at a method definition > and instantly know what arguments are mutable. > > Adding "final" on fields, methods and classes has a far greater effect > than that of method arguments. If we intent to enforce it based on > readability, then we should really consider applying different approaches > (as stated in the paragraph above) that can be used with far greater > effectiveness. > > Longer reasoning: > > I think all arguments relating to "indicating authors intent" or > "variable does not change" are great, but in reality we as developers > should always assume that this was the intent. I think if we really want to > improve our code base, let's opt for IMMUTABLE data objects. The "final" > keyword has no benefit if you pass around POJOs (including Collections). As > long as you don't change the instance reference, the "final" keyword has > absolutely no effect. It does not mean that you cannot change every field > internal of the POJO. The "final" keyword is only effective against the > re-assignment of that variable/argument within a method. But a better > practice is to avoid the re-assignment of any variable/argument in the > first place. > > Kotlin has a great language (one of many) that method/function > arguments are naturally final and this behavior cannot be changed. In > addition there is a the feature that the collections are immutable and as a > developer you have to be explicit if you want a collection to be mutable, > by defining the collection as a "Mutable*" i.e MutableMap, MutableSet, > MutableList > > @Jake the idea of smaller methods is great and we should ALWAYS strive > for that. But that argument is completely irrelevant in this discussion. As > making method arguments final does not naturally guide a developer to > creating smaller methods. Nor does a smaller method mean it can/will be > jitted. Too many factors (to discuss here) are part of that decision, also > it is not relevant in this discussion. But more on that topic read THIS. > > --Udo > > On 4/15/21, 9:29 AM, "Jacob Barrett" wrote: > > If a method is longer than a handful of lines and I go in to > refactor it I am going to start by making every variable I find final. Then > I am going to figure out how to keep them final. By doing so you naturally > produce smaller functional methods that are usually independently unit > testable. Smaller methods can get jitted. Smaller methods can take less > time to reach a safe point (see time to safe point issues in Java). > > For this reason I strongly prefer on final everywhere and if you > think you need a variable to not be final you should use that as an > indicator to refractor your code. > > So, +1 for me for final everywhere. > > Also, please stop declaring variables miles away from their use. > > > On Apr 14, 2021, at 12:55 PM, Kirk Lund > 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 vot
[DISCUSS] Requiring final keyword on every parameter and local variable
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
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 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
PS: Here is my version of that constructor after refactoring the class:
PartitionedRegionClearMessage(Set 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 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 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: [VOTE] Requiring final keyword on every parameter and local variable
@jake, you are correct, I did miss the LOCAL variable out of the subject. :face_palm: Yes, adding "final" to LOCAL variables will be HUGELY beneficial in this regard. Have we seen any performance improvement after having refactored this method? Have you been able to measure any improvements? --Udo On 4/15/21, 1:19 PM, "Jacob Barrett" wrote: > On Apr 14, 2021, at 7:46 PM, Udo Kohlmeyer wrote: > @Jake the idea of smaller methods is great and we should ALWAYS strive for that. But that argument is completely irrelevant in this discussion. As making method arguments final does not naturally guide a developer to creating smaller methods. Nor does a smaller method mean it can/will be jitted. Too many factors (to discuss here) are part of that decision, also it is not relevant in this discussion. But more on that topic read THIS. The original subject is in regards to parameters and local variables. Irrelevant is certainly an opinion you are welcome to have but let me challenge you. Goto DistributedCacheOperation._distribute(). First challenge, look at around line 333: boolean reliableOp = isOperationReliable() && region.requiresReliabilityCheck(); Without scrolling do you see that variable used? Nope, because it is first used on line 439, ~100 lines away. Does it mutate between there, well I can search for all uses and find out or I could be nice to the next person and intend for it to never mutate by adding final. Intent communicated! Second challenge, mark all the local variables in the method as final. Now make it compile without introducing more mutable variables. At the end of this journey you will have about a dozen unit testable methods and a _distribute method that goes from ~370 lines to ~90 with no mutable local variables. I argue it is relevant as good guardrail for writing good code. While we should ALWAYS strive for it we don’t. Every little nudge helps. -Jake
Re: [VOTE] Requiring final keyword on every parameter and local variable
> On Apr 15, 2021, at 7:49 PM, Udo Kohlmeyer wrote: > > @jake, you are correct, I did miss the LOCAL variable out of the subject. > :face_palm: > > Yes, adding "final" to LOCAL variables will be HUGELY beneficial in this > regard. Have we seen any performance improvement after having refactored this > method? Have you been able to measure any improvements? I haven’t finished yet. I need to remove the multiple hash map copies in there. I have a few more smaller methods to pull out. Then I think I can get most of it, at least the happy path, to JIT. So close! -Jake
