You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Kirk Lund <kl...@apache.org> on 2021/04/14 19:55:49 UTC
[VOTE] Requiring final keyword on every parameter and local variable
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
Posted by Patrick Johnson <jp...@vmware.com>.
-1 for requiring final for all method parameters. While I have nothing against using final here, I don’t think it’s worth mandating and would be better left up to the individual and the specific circumstance.
On Apr 14, 2021, at 4:33 PM, Jason Huynh <jh...@vmware.com>> wrote:
Yeah, understood just an opinion about noise __ The clarity and precision part I'm debating a bit but I also don't do code review or work in the actual code base as much as before so I'll go with a -0 vote for me and let everyone else that does see the code more often decide __
On 4/14/21, 4:28 PM, "Robert Houghton" <rh...@vmware.com>> wrote:
Does this really "add noise" instead of "add clarity and precision" ?
-Robert
On 4/14/21, 4:25 PM, "Jason Huynh" <jh...@vmware.com>> wrote:
Just to confirm for myself... final doesn't necessarily make object immutable.. the reference is immutable but if it refers to say a list, the list is still mutable.
So if I see a final keyword, I should only expect the reference not to change, but it won't guarantee that the object isn't changing.
I'm a -1 on forcing the issue because I believe we currently have too many "edge cases" where we can't stick a final on. So we end up adding complexity in reading without the benefit guaranteeing anything... I'd dislike seeing something like this make it partway and then get abandoned...
Also are we only discussing local variables, class variables and method parameters? Ignoring final on methods and classes?
On 4/14/21, 2:18 PM, "Mark Hanson" <ha...@vmware.com>> wrote:
+1 To Kirk’s approach.
-1 To final everywhere.
I support Kirk’s approach of adding final where there is benefit. If we want to go further, we can have intellij find all of them and change them if we want. Final is not always a developer’s explicit statement. Sometimes something becomes final after the fact.
If we manually touch any code, I would much rather we deal with the use of raw types than have final everywhere. This strikes me as a minor issue compared to the rampant use of raw types.
E.g.
RegionFactory regionFactory = getCache().createRegionFactory(RegionShortcut.PARTITION_REDUNDANT);
Region region = regionFactory.create (“testRegion”);
Region.put(int, String);`
Thanks,
Mark
From: Bill Burcham <bi...@gmail.com>>
Date: Wednesday, April 14, 2021 at 1:53 PM
To: dev@geode.apache.org<ma...@geode.apache.org> <de...@geode.apache.org>>
Subject: Re: [VOTE] Requiring final keyword on every parameter and local variable
+1
I am 100% for putting final everywhere it is possible to put it. Call this
the "hard final" position.
What I've been advocating for, intermittently, in PRs (for example, in PR
#6310 <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6310%23discussion_r612710322&data=04%7C01%7Cjpatrick%40vmware.com%7C5df09b1eacad48722f0708d8ff9db7d5%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637540400159396125%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Oz4syWeYiLGnZTPg7YFGgCvT7kqVu4J%2BTfgDvYN0Qa4%3D&reserved=0>) is
more of a "soft final" position which I can formulate as:
The final keyword is valuable everywhere. When we encounter it in code, we
assume that the author put it there intentionally to communicate that the
variable will not change. Since the code compiled before our change, we
know that the variable does not change (the compiler has proven it!) When
modifying code, we do not remove the final keyword unless we have to (to
get the code to compile).
So that's the "soft final" position. But I'm all for Kirk's "hard final" if
others agree. I just don't see any downside—only upside on this one.
The final keyword is a vehicle, like types, for programmers to communicate
their intentions. In both cases, the compiler can provide valuable
validation. There is, unfortunately, some visual noise entailed (lots of
occurrences of "final" in the code). But this makes non-final (mutable)
variables much more apparent. Since they are (or should be) in the
minority, this is an aid to maintainers. This becomes apparent in longer,
more complex methods.
On Wed, Apr 14, 2021 at 12:56 PM Kirk Lund <kl...@apache.org>> 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
Posted by Jason Huynh <jh...@vmware.com>.
Yeah, understood just an opinion about noise __ The clarity and precision part I'm debating a bit but I also don't do code review or work in the actual code base as much as before so I'll go with a -0 vote for me and let everyone else that does see the code more often decide __
On 4/14/21, 4:28 PM, "Robert Houghton" <rh...@vmware.com> wrote:
Does this really "add noise" instead of "add clarity and precision" ?
-Robert
On 4/14/21, 4:25 PM, "Jason Huynh" <jh...@vmware.com> wrote:
Just to confirm for myself... final doesn't necessarily make object immutable.. the reference is immutable but if it refers to say a list, the list is still mutable.
So if I see a final keyword, I should only expect the reference not to change, but it won't guarantee that the object isn't changing.
I'm a -1 on forcing the issue because I believe we currently have too many "edge cases" where we can't stick a final on. So we end up adding complexity in reading without the benefit guaranteeing anything... I'd dislike seeing something like this make it partway and then get abandoned...
Also are we only discussing local variables, class variables and method parameters? Ignoring final on methods and classes?
On 4/14/21, 2:18 PM, "Mark Hanson" <ha...@vmware.com> wrote:
+1 To Kirk’s approach.
-1 To final everywhere.
I support Kirk’s approach of adding final where there is benefit. If we want to go further, we can have intellij find all of them and change them if we want. Final is not always a developer’s explicit statement. Sometimes something becomes final after the fact.
If we manually touch any code, I would much rather we deal with the use of raw types than have final everywhere. This strikes me as a minor issue compared to the rampant use of raw types.
E.g.
RegionFactory regionFactory = getCache().createRegionFactory(RegionShortcut.PARTITION_REDUNDANT);
Region region = regionFactory.create (“testRegion”);
Region.put(int, String);`
Thanks,
Mark
From: Bill Burcham <bi...@gmail.com>
Date: Wednesday, April 14, 2021 at 1:53 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: [VOTE] Requiring final keyword on every parameter and local variable
+1
I am 100% for putting final everywhere it is possible to put it. Call this
the "hard final" position.
What I've been advocating for, intermittently, in PRs (for example, in PR
#6310 <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6310%23discussion_r612710322&data=04%7C01%7Cjhuynh%40vmware.com%7Cfd64a4ac6e3b450cd1a508d8ff9d0a44%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637540397240628225%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=CpfN5mam1U5TNNWIPX8%2BLx8ZHp80u78nKogPDp9Q%2BQU%3D&reserved=0>) is
more of a "soft final" position which I can formulate as:
The final keyword is valuable everywhere. When we encounter it in code, we
assume that the author put it there intentionally to communicate that the
variable will not change. Since the code compiled before our change, we
know that the variable does not change (the compiler has proven it!) When
modifying code, we do not remove the final keyword unless we have to (to
get the code to compile).
So that's the "soft final" position. But I'm all for Kirk's "hard final" if
others agree. I just don't see any downside—only upside on this one.
The final keyword is a vehicle, like types, for programmers to communicate
their intentions. In both cases, the compiler can provide valuable
validation. There is, unfortunately, some visual noise entailed (lots of
occurrences of "final" in the code). But this makes non-final (mutable)
variables much more apparent. Since they are (or should be) in the
minority, this is an aid to maintainers. This becomes apparent in longer,
more complex methods.
On Wed, Apr 14, 2021 at 12:56 PM Kirk Lund <kl...@apache.org> 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
Posted by Robert Houghton <rh...@vmware.com>.
Does this really "add noise" instead of "add clarity and precision" ?
-Robert
On 4/14/21, 4:25 PM, "Jason Huynh" <jh...@vmware.com> wrote:
Just to confirm for myself... final doesn't necessarily make object immutable.. the reference is immutable but if it refers to say a list, the list is still mutable.
So if I see a final keyword, I should only expect the reference not to change, but it won't guarantee that the object isn't changing.
I'm a -1 on forcing the issue because I believe we currently have too many "edge cases" where we can't stick a final on. So we end up adding complexity in reading without the benefit guaranteeing anything... I'd dislike seeing something like this make it partway and then get abandoned...
Also are we only discussing local variables, class variables and method parameters? Ignoring final on methods and classes?
On 4/14/21, 2:18 PM, "Mark Hanson" <ha...@vmware.com> wrote:
+1 To Kirk’s approach.
-1 To final everywhere.
I support Kirk’s approach of adding final where there is benefit. If we want to go further, we can have intellij find all of them and change them if we want. Final is not always a developer’s explicit statement. Sometimes something becomes final after the fact.
If we manually touch any code, I would much rather we deal with the use of raw types than have final everywhere. This strikes me as a minor issue compared to the rampant use of raw types.
E.g.
RegionFactory regionFactory = getCache().createRegionFactory(RegionShortcut.PARTITION_REDUNDANT);
Region region = regionFactory.create (“testRegion”);
Region.put(int, String);`
Thanks,
Mark
From: Bill Burcham <bi...@gmail.com>
Date: Wednesday, April 14, 2021 at 1:53 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: [VOTE] Requiring final keyword on every parameter and local variable
+1
I am 100% for putting final everywhere it is possible to put it. Call this
the "hard final" position.
What I've been advocating for, intermittently, in PRs (for example, in PR
#6310 <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6310%23discussion_r612710322&data=04%7C01%7Crhoughton%40vmware.com%7C07622d5b63e947f8ae8a08d8ff9c9761%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637540395310059043%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=cPBS4vgpCEBxmLaV%2B8hlkx5xONw6R9bln32riw42d%2Bg%3D&reserved=0>) is
more of a "soft final" position which I can formulate as:
The final keyword is valuable everywhere. When we encounter it in code, we
assume that the author put it there intentionally to communicate that the
variable will not change. Since the code compiled before our change, we
know that the variable does not change (the compiler has proven it!) When
modifying code, we do not remove the final keyword unless we have to (to
get the code to compile).
So that's the "soft final" position. But I'm all for Kirk's "hard final" if
others agree. I just don't see any downside—only upside on this one.
The final keyword is a vehicle, like types, for programmers to communicate
their intentions. In both cases, the compiler can provide valuable
validation. There is, unfortunately, some visual noise entailed (lots of
occurrences of "final" in the code). But this makes non-final (mutable)
variables much more apparent. Since they are (or should be) in the
minority, this is an aid to maintainers. This becomes apparent in longer,
more complex methods.
On Wed, Apr 14, 2021 at 12:56 PM Kirk Lund <kl...@apache.org> 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
Posted by Jason Huynh <jh...@vmware.com>.
Just to confirm for myself... final doesn't necessarily make object immutable.. the reference is immutable but if it refers to say a list, the list is still mutable.
So if I see a final keyword, I should only expect the reference not to change, but it won't guarantee that the object isn't changing.
I'm a -1 on forcing the issue because I believe we currently have too many "edge cases" where we can't stick a final on. So we end up adding complexity in reading without the benefit guaranteeing anything... I'd dislike seeing something like this make it partway and then get abandoned...
Also are we only discussing local variables, class variables and method parameters? Ignoring final on methods and classes?
On 4/14/21, 2:18 PM, "Mark Hanson" <ha...@vmware.com> wrote:
+1 To Kirk’s approach.
-1 To final everywhere.
I support Kirk’s approach of adding final where there is benefit. If we want to go further, we can have intellij find all of them and change them if we want. Final is not always a developer’s explicit statement. Sometimes something becomes final after the fact.
If we manually touch any code, I would much rather we deal with the use of raw types than have final everywhere. This strikes me as a minor issue compared to the rampant use of raw types.
E.g.
RegionFactory regionFactory = getCache().createRegionFactory(RegionShortcut.PARTITION_REDUNDANT);
Region region = regionFactory.create (“testRegion”);
Region.put(int, String);`
Thanks,
Mark
From: Bill Burcham <bi...@gmail.com>
Date: Wednesday, April 14, 2021 at 1:53 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: [VOTE] Requiring final keyword on every parameter and local variable
+1
I am 100% for putting final everywhere it is possible to put it. Call this
the "hard final" position.
What I've been advocating for, intermittently, in PRs (for example, in PR
#6310 <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6310%23discussion_r612710322&data=04%7C01%7Cjhuynh%40vmware.com%7Cb302f4697dfc4bcd8b1a08d8ff8acb73%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637540318872199677%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=PMLWK78eNleFFqziFRt43H56KHJK0H7wo7OqQ%2BMwiAI%3D&reserved=0>) is
more of a "soft final" position which I can formulate as:
The final keyword is valuable everywhere. When we encounter it in code, we
assume that the author put it there intentionally to communicate that the
variable will not change. Since the code compiled before our change, we
know that the variable does not change (the compiler has proven it!) When
modifying code, we do not remove the final keyword unless we have to (to
get the code to compile).
So that's the "soft final" position. But I'm all for Kirk's "hard final" if
others agree. I just don't see any downside—only upside on this one.
The final keyword is a vehicle, like types, for programmers to communicate
their intentions. In both cases, the compiler can provide valuable
validation. There is, unfortunately, some visual noise entailed (lots of
occurrences of "final" in the code). But this makes non-final (mutable)
variables much more apparent. Since they are (or should be) in the
minority, this is an aid to maintainers. This becomes apparent in longer,
more complex methods.
On Wed, Apr 14, 2021 at 12:56 PM Kirk Lund <kl...@apache.org> 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
Posted by Mark Hanson <ha...@vmware.com>.
+1 To Kirk’s approach.
-1 To final everywhere.
I support Kirk’s approach of adding final where there is benefit. If we want to go further, we can have intellij find all of them and change them if we want. Final is not always a developer’s explicit statement. Sometimes something becomes final after the fact.
If we manually touch any code, I would much rather we deal with the use of raw types than have final everywhere. This strikes me as a minor issue compared to the rampant use of raw types.
E.g.
RegionFactory regionFactory = getCache().createRegionFactory(RegionShortcut.PARTITION_REDUNDANT);
Region region = regionFactory.create (“testRegion”);
Region.put(int, String);`
Thanks,
Mark
From: Bill Burcham <bi...@gmail.com>
Date: Wednesday, April 14, 2021 at 1:53 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: [VOTE] Requiring final keyword on every parameter and local variable
+1
I am 100% for putting final everywhere it is possible to put it. Call this
the "hard final" position.
What I've been advocating for, intermittently, in PRs (for example, in PR
#6310 <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6310%23discussion_r612710322&data=04%7C01%7Chansonm%40vmware.com%7C2d04f6e8170147093c5408d8ff875e13%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637540304156955021%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=ljAtQngyDiOvpcQcq86Xuctt3LBK3nmlXkt3LyO3dYg%3D&reserved=0>) is
more of a "soft final" position which I can formulate as:
The final keyword is valuable everywhere. When we encounter it in code, we
assume that the author put it there intentionally to communicate that the
variable will not change. Since the code compiled before our change, we
know that the variable does not change (the compiler has proven it!) When
modifying code, we do not remove the final keyword unless we have to (to
get the code to compile).
So that's the "soft final" position. But I'm all for Kirk's "hard final" if
others agree. I just don't see any downside—only upside on this one.
The final keyword is a vehicle, like types, for programmers to communicate
their intentions. In both cases, the compiler can provide valuable
validation. There is, unfortunately, some visual noise entailed (lots of
occurrences of "final" in the code). But this makes non-final (mutable)
variables much more apparent. Since they are (or should be) in the
minority, this is an aid to maintainers. This becomes apparent in longer,
more complex methods.
On Wed, Apr 14, 2021 at 12:56 PM Kirk Lund <kl...@apache.org> 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
Posted by Bill Burcham <bi...@gmail.com>.
+1
I am 100% for putting final everywhere it is possible to put it. Call this
the "hard final" position.
What I've been advocating for, intermittently, in PRs (for example, in PR
#6310 <https://github.com/apache/geode/pull/6310#discussion_r612710322>) is
more of a "soft final" position which I can formulate as:
The final keyword is valuable everywhere. When we encounter it in code, we
assume that the author put it there intentionally to communicate that the
variable will not change. Since the code compiled before our change, we
know that the variable does not change (the compiler has proven it!) When
modifying code, we do not remove the final keyword unless we have to (to
get the code to compile).
So that's the "soft final" position. But I'm all for Kirk's "hard final" if
others agree. I just don't see any downside—only upside on this one.
The final keyword is a vehicle, like types, for programmers to communicate
their intentions. In both cases, the compiler can provide valuable
validation. There is, unfortunately, some visual noise entailed (lots of
occurrences of "final" in the code). But this makes non-final (mutable)
variables much more apparent. Since they are (or should be) in the
minority, this is an aid to maintainers. This becomes apparent in longer,
more complex methods.
On Wed, Apr 14, 2021 at 12:56 PM Kirk Lund <kl...@apache.org> 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
Posted by Kirk Lund <kl...@apache.org>.
It's past the announced deadline so I've counted the results of our vote to
require 'final' keyword on all local variables and method/constructor
parameters.
I am interpreting this vote as a procedural issue [1], and I counted votes
by PMC members as "binding votes" and votes by all other participants as
"advisory votes" [2]. Please correct me if I've done any of this
incorrectly, and we'll try to get the results fixed.
Given that I stumbled during the beginning and started the VOTE before the
DISCUSS, I'm happy to start the process over if anyone believes that we
should repeat it. I've taken notes and I'm convinced I'll follow the
process correctly next time.
Voting status
=============
+1: 2 binding votes
* Bill Burcham (PMC member)
* Jacob Barrett (PMC member)
+0: zero votes
-0: 1 binding vote, 1 advisory vote
* Jason Huynh (PMC member)
* Dale Emery (committer)
-1: 4 binding votes, 1 advisory vote
* Mark Hanson (PMC member)
* Patrick Johnson (contributor)
* Udo Kohlmeyer (PMC member)
* Kirk Lund (PMC member)
* John Blum (PMC member)
* Bruce Schuchardt (PMC member) - sorry, vote was past deadline
The voting fails with a majority of -1 votes.
[1] https://www.apache.org/foundation/voting.html
[2] https://projects.apache.org/committee.html?geode
Thanks everyone,
Kirk
(PS: should this result email be part of the original VOTE thread or should
it start a new thread for results?)
On Thu, Apr 22, 2021 at 9:53 AM Kirk Lund <kl...@apache.org> wrote:
> Voting and discussion are closed for now. I'll count up the votes and post
> a summary soon.
>
> -Kirk
>
> On Thu, Apr 22, 2021 at 7:54 AM Bruce Schuchardt <br...@vmware.com>
> wrote:
>
>> Kirk speaks my mind. -1 on this requirement
>>
>> On 4/21/21, 11:29 AM, "Kirk Lund" <kl...@apache.org> wrote:
>>
>> -1 as I've already explained in my DISCUSS thread for this topic, I
>> don't
>> think final gives sufficient benefit for local variables or method
>> parameters because it only prevents reassigning object pointers and
>> primitive values.
>>
>> I favor individuals making a personal decision on whether or not they
>> want
>> to use 'final' in this way without requiring others to do the same
>> (and NOT
>> requiring this use of 'final' in PR reviews, although "suggesting" is
>> ok).
>>
>> Note that JDK and all other libraries I looked through do not use
>> final in
>> this way.
>>
>> *I'm willing to change my vote to -0 if*:
>>
>> 1. we qualify the requirement to only apply to method and
>> constructor
>> parameters (not local vars)
>> 2. we find a way to introduce automation for enforcement (such as
>> we do
>> with RAT for license headers)
>>
>> *I'm willing to change my vote to +1 if *we decide to require 'final'
>> parameters (and even local vars) as part of a bigger shift towards
>> improving code quality by embracing Clean Code (ala Bob Martin):
>>
>> 1. we commit to developing a robust and thorough coding standard
>> that
>> includes requiring true unit tests and passing in dependencies via
>> class
>> constructors or methods
>> 2. we revisit our DOD and enforce it in PR reviews
>>
>> I recommend that contributors use IntelliJ inspections (or comparable
>> guideline for anyone who doesn't want to use IntelliJ) that will
>> assist us
>> in avoiding and fixing long methods when writing new methods and when
>> editing/refactoring existing methods (see below)
>> Method metrics (see IntelliJ > Preferences > Editor > Inspections):
>>
>> - Method with too many parameters
>> - Overly complex method
>> - Overly long method
>> - Overly nested method
>>
>> The inspections can be customized for Geode as long as the values
>> aren't
>> increased to the point of it being useless. For example, we would
>> have to
>> agree as a community what the "maximum number of parameters" is for
>> the
>> "Method with too many parameters" inspection.
>>
>> We could even export an inspections profile to be stored in the Geode
>> repo
>> and used by contributors.
>>
>> -Kirk
>>
>> On Wed, Apr 14, 2021 at 12:55 PM Kirk Lund <kl...@apache.org> 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
Posted by Kirk Lund <kl...@apache.org>.
Voting and discussion are closed for now. I'll count up the votes and post
a summary soon.
-Kirk
On Thu, Apr 22, 2021 at 7:54 AM Bruce Schuchardt <br...@vmware.com> wrote:
> Kirk speaks my mind. -1 on this requirement
>
> On 4/21/21, 11:29 AM, "Kirk Lund" <kl...@apache.org> wrote:
>
> -1 as I've already explained in my DISCUSS thread for this topic, I
> don't
> think final gives sufficient benefit for local variables or method
> parameters because it only prevents reassigning object pointers and
> primitive values.
>
> I favor individuals making a personal decision on whether or not they
> want
> to use 'final' in this way without requiring others to do the same
> (and NOT
> requiring this use of 'final' in PR reviews, although "suggesting" is
> ok).
>
> Note that JDK and all other libraries I looked through do not use
> final in
> this way.
>
> *I'm willing to change my vote to -0 if*:
>
> 1. we qualify the requirement to only apply to method and
> constructor
> parameters (not local vars)
> 2. we find a way to introduce automation for enforcement (such as
> we do
> with RAT for license headers)
>
> *I'm willing to change my vote to +1 if *we decide to require 'final'
> parameters (and even local vars) as part of a bigger shift towards
> improving code quality by embracing Clean Code (ala Bob Martin):
>
> 1. we commit to developing a robust and thorough coding standard
> that
> includes requiring true unit tests and passing in dependencies via
> class
> constructors or methods
> 2. we revisit our DOD and enforce it in PR reviews
>
> I recommend that contributors use IntelliJ inspections (or comparable
> guideline for anyone who doesn't want to use IntelliJ) that will
> assist us
> in avoiding and fixing long methods when writing new methods and when
> editing/refactoring existing methods (see below)
> Method metrics (see IntelliJ > Preferences > Editor > Inspections):
>
> - Method with too many parameters
> - Overly complex method
> - Overly long method
> - Overly nested method
>
> The inspections can be customized for Geode as long as the values
> aren't
> increased to the point of it being useless. For example, we would have
> to
> agree as a community what the "maximum number of parameters" is for the
> "Method with too many parameters" inspection.
>
> We could even export an inspections profile to be stored in the Geode
> repo
> and used by contributors.
>
> -Kirk
>
> On Wed, Apr 14, 2021 at 12:55 PM Kirk Lund <kl...@apache.org> 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
Posted by Bruce Schuchardt <br...@vmware.com>.
Kirk speaks my mind. -1 on this requirement
On 4/21/21, 11:29 AM, "Kirk Lund" <kl...@apache.org> wrote:
-1 as I've already explained in my DISCUSS thread for this topic, I don't
think final gives sufficient benefit for local variables or method
parameters because it only prevents reassigning object pointers and
primitive values.
I favor individuals making a personal decision on whether or not they want
to use 'final' in this way without requiring others to do the same (and NOT
requiring this use of 'final' in PR reviews, although "suggesting" is ok).
Note that JDK and all other libraries I looked through do not use final in
this way.
*I'm willing to change my vote to -0 if*:
1. we qualify the requirement to only apply to method and constructor
parameters (not local vars)
2. we find a way to introduce automation for enforcement (such as we do
with RAT for license headers)
*I'm willing to change my vote to +1 if *we decide to require 'final'
parameters (and even local vars) as part of a bigger shift towards
improving code quality by embracing Clean Code (ala Bob Martin):
1. we commit to developing a robust and thorough coding standard that
includes requiring true unit tests and passing in dependencies via class
constructors or methods
2. we revisit our DOD and enforce it in PR reviews
I recommend that contributors use IntelliJ inspections (or comparable
guideline for anyone who doesn't want to use IntelliJ) that will assist us
in avoiding and fixing long methods when writing new methods and when
editing/refactoring existing methods (see below)
Method metrics (see IntelliJ > Preferences > Editor > Inspections):
- Method with too many parameters
- Overly complex method
- Overly long method
- Overly nested method
The inspections can be customized for Geode as long as the values aren't
increased to the point of it being useless. For example, we would have to
agree as a community what the "maximum number of parameters" is for the
"Method with too many parameters" inspection.
We could even export an inspections profile to be stored in the Geode repo
and used by contributors.
-Kirk
On Wed, Apr 14, 2021 at 12:55 PM Kirk Lund <kl...@apache.org> 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
Posted by Kirk Lund <kl...@apache.org>.
-1 as I've already explained in my DISCUSS thread for this topic, I don't
think final gives sufficient benefit for local variables or method
parameters because it only prevents reassigning object pointers and
primitive values.
I favor individuals making a personal decision on whether or not they want
to use 'final' in this way without requiring others to do the same (and NOT
requiring this use of 'final' in PR reviews, although "suggesting" is ok).
Note that JDK and all other libraries I looked through do not use final in
this way.
*I'm willing to change my vote to -0 if*:
1. we qualify the requirement to only apply to method and constructor
parameters (not local vars)
2. we find a way to introduce automation for enforcement (such as we do
with RAT for license headers)
*I'm willing to change my vote to +1 if *we decide to require 'final'
parameters (and even local vars) as part of a bigger shift towards
improving code quality by embracing Clean Code (ala Bob Martin):
1. we commit to developing a robust and thorough coding standard that
includes requiring true unit tests and passing in dependencies via class
constructors or methods
2. we revisit our DOD and enforce it in PR reviews
I recommend that contributors use IntelliJ inspections (or comparable
guideline for anyone who doesn't want to use IntelliJ) that will assist us
in avoiding and fixing long methods when writing new methods and when
editing/refactoring existing methods (see below)
Method metrics (see IntelliJ > Preferences > Editor > Inspections):
- Method with too many parameters
- Overly complex method
- Overly long method
- Overly nested method
The inspections can be customized for Geode as long as the values aren't
increased to the point of it being useless. For example, we would have to
agree as a community what the "maximum number of parameters" is for the
"Method with too many parameters" inspection.
We could even export an inspections profile to be stored in the Geode repo
and used by contributors.
-Kirk
On Wed, Apr 14, 2021 at 12:55 PM Kirk Lund <kl...@apache.org> 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
Posted by Mark Hanson <ha...@vmware.com>.
Does everyone feel that adding final here is the best thing to do? If not, then "Always" becomes "Strongly Encouraged where appropriate"
protected void validateUDPEncryptionStats() {
long encrptTime = getGemfireCache().getDistributionManager().getStats().getUDPMsgEncryptionTime();
long decryptTime = getGemfireCache().getDistributionManager().getStats().getUDPMsgDecryptionTime();
assertTrue("Should have multicast writes or reads. encrptTime= " + encrptTime
+ " ,decryptTime= " + decryptTime, encrptTime == 0 && decryptTime == 0);
}
Note: the code actually warns about getGemfireCache() being deprecated.
Thanks,
Mark
On 4/19/21, 11:08 AM, "Mark Hanson" <ha...@vmware.com> wrote:
Hi Jake,
I agree with everyone's point about final being a useful, I just don't find it useful enough to do anything manually across the code base at this point.
I believe first in foremost in no code warnings. Intellij warns about variables that can be final, so I make them final as it finds them. It is very rare that I am writing new code at this point. I have spent the last year bug fixing other people's code. From my standpoint, original intent is largely moot. So, for me, the question is do I go through code that is not mine and mark it all as final (where appropriate)? Sure, in code that I touch. In code the rest of the codebase, I think there are bigger fish to fry before we get to final.
It seems like the larger portion of the consensus is to recommend that variables are marked final (where appropriate) as we find them or create them.
That seems like the going forward approach consensus.
I will be blunt though. This seems nitpicky *compared* to the number of code warnings there are and the fact that people are not actively fixing all of the warnings.
If we don't come to the same consensus about all warnings this seems like painting a rusted car, sure it makes it all look the same, but does very little for the underlying problems. Now to undermine my own argument a little, I think that especially in release blockers, we want to touch as little code as possible, so I am flexible there.
I would also like to agree with Udo about final really not being very good compared to Mutable and Immutable objects in other languages.
Thanks,
Mark
On 4/15/21, 7:49 PM, "Udo Kohlmeyer" <ud...@vmware.com> 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?
--Udo
On 4/15/21, 1:19 PM, "Jacob Barrett" <ja...@vmware.com> wrote:
> On Apr 14, 2021, at 7:46 PM, Udo Kohlmeyer <ud...@vmware.com> 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
Posted by John Blum <jb...@vmware.com>.
Argy! Sorry typo (corrected with surrounding ***)...
I vote -1 for (generally) using final within constructor/method parameters and local variables.
While final (e.g. on instance variables) has guaranteed benefits during construction (e.g. initialization safety of an object used in a muti-Threaded context) as Kirk rightly pointed out, its non-apparent, intended use can be ill-conceived and ill-received outside this context.
As Jason also mentioned, which cannot be overemphasized enough, final (by itself) does not guarantee immutability, which is often the intention of using final in the first place.
While using final ensures immutability of primitive types (e.g. int, & their corresponding wrapper types, i.e. Integer, only because the Integer class is immutable to begin with) it does not ensure immutability of reference types (***mutable objects***), arrays or Collection types. You can still change primitive arrays or arrays/Collections containing reference types, even when the array/Collection reference is final, regardless of whether an array/Collection is passed to a constructor or method. Additionally, arrays and Collections may contain mutable objects.
While there is some utility in using final, there are often much better techniques that can be applied, such as, but not limited to:
1) Strict validation and read-only use of arguments (effectively immutable).
2) Making defensive copies of arguments passed in where appropriate.
3) Limiting the scope of the (defined) variable. E.g. Using (private) utility methods to limit the scope and use (processing) of the variable passed into a constructor or method as an argument.
Keep in mind that Java is Pass by Value, not Pass by Reference, and in some cases, you can use this fact to your advantage.
So, because final is not a complete solution, this is why I vote -1.
NOTE: This does not mean I don't support using final in some cases (even for constructor or method parameters and even local variables), just that it should be used judiciously, like everything else, to explicitly express intent. And, most importantly, function should match intent.
-j
________________________________
From: John Blum <jb...@vmware.com>
Sent: Wednesday, April 21, 2021 6:02 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: [VOTE] Requiring final keyword on every parameter and local variable
I vote -1 for (generally) using final within constructor/method parameters and local variables.
While final (e.g. on instance variables) has guaranteed benefits during construction (e.g. initialization safety of an object used in a muti-Threaded context) as Kirk rightly pointed out, its non-apparent, intended use can be ill-conceived and ill-received outside this context.
As Jason also mentioned, which cannot be overemphasized enough, final (by itself) does not guarantee immutability, which is often the intention of using final in the first place.
While using final ensures immutability of primitive types (e.g. int, & their corresponding wrapper types, i.e. Integer, only because the Integer class is immutable to begin with) it does not ensure immutability of reference types (immutable objects), arrays or Collection types. You can still change primitive arrays or arrays/Collections containing reference types, even when the array/Collection reference is final, regardless of whether an array/Collection is passed to a constructor or method. Additionally, arrays and Collections may contain mutable objects.
While there is some utility in using final, there are often much better techniques that can be applied, such as, but not limited to:
1) Strict validation and read-only use of arguments (effectively immutable).
2) Making defensive copies of arguments passed in where appropriate.
3) Limiting the scope of the (defined) variable. E.g. Using (private) utility methods to limit the scope and use (processing) of the variable passed into a constructor or method as an argument.
Keep in mind that Java is Pass by Value, not Pass by Reference, and in some cases, you can use this fact to your advantage.
So, because final is not a complete solution, this is why I vote -1.
NOTE: This does not mean I don't support using final in some cases (even for constructor or method parameters and even local variables), just that it should be used judiciously, like everything else, to explicitly express intent. And, most importantly, function should match intent.
-j
________________________________
From: Owen Nichols <on...@vmware.com>
Sent: Wednesday, April 21, 2021 5:16 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: [VOTE] Requiring final keyword on every parameter and local variable
Good point. We're probably stuck on vanilla JDK8 for years more anyway.
Since this discussion seems to be mainly about personal viewing preference, I wonder if someone has written an IntellJ plugin that would simply render effectively-final variables as if they were preceded by the word "final" (for people that want to see finals everywhere), or a plugin to collapse unnecessary finals (for people that don't want to see them).
On 4/21/21, 4:36 PM, "John Blum" <jb...@vmware.com> wrote:
I wouldn't recommend Lombok for production code, primarily because it generates code. Generated code occasionally leads to unintended consequences or incompatibilities with newer Java versions. Additionally, it requires as 3rd-party dependency to gain these capabilities rather than being part of the Java language itself. I'd rather Java include appropriate features for these 2 concerns, i.e. val & var, much like Kotlin, to more succinctly express intent. Even though Java now has var, it is different than Kotlin's var.
I only use Lombok for testing and demoing purposes.
-j
________________________________
From: Kirk Lund <kl...@apache.org>
Sent: Wednesday, April 21, 2021 12:59 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: [VOTE] Requiring final keyword on every parameter and local variable
I'm interested in https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprojectlombok.org%2F&data=04%7C01%7Cjblum%40vmware.com%7C289192b0f0e1448ab80008d9052a5047%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637546501568451216%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=C5enkn9VDMUBjDtyJN3PTTDZyy0MGeLzWvz1dv6srGs%3D&reserved=0 and I remember John Blum using
it on some projects. I would like to study it and do some perf testing on
it before supporting it for Geode though. In general, I don't like
generated code and it looks like at least some of the features involve
generated code -- that's the only potential downside for me.
-Kirk
On Mon, Apr 19, 2021 at 12:57 PM Owen Nichols <on...@vmware.com> wrote:
> Any interest in adopting https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprojectlombok.org%2F&data=04%7C01%7Cjblum%40vmware.com%7C289192b0f0e1448ab80008d9052a5047%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637546501568461213%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=TCcWHcQaOOcpZ4n7s%2Bgrl4RzHmR8tc40%2B1Mp7Yi6%2F%2Bw%3D&reserved=0 in Geode? Lombok
> allow a more readable "val" vs "var" syntax instead of "final" vs "" which
> could make it easier to do the right thing without "increasing visual
> noise".
>
> On 4/19/21, 12:40 PM, "Dale Emery" <de...@vmware.com> wrote:
>
> My test for whether enforce a guideline in a PR review is: Would I be
> willing to automate enforcing it in CI?
>
> I am -0 on this particular guideline.
>
> IntelliJ offers two competing inspections for Java coding style:
>
> * Local variable or parameter can be final
> * Unnecessary 'final' on local variable or parameter
>
> Both of these are (I think) disabled by default in IntelliJ. And
> neither satisfies my “I’d be willing to automate enforcing it” test.
>
> Dale
>
> From: Mark Hanson <ha...@vmware.com>
> Date: Monday, April 19, 2021 at 11:08 AM
> To: dev@geode.apache.org <de...@geode.apache.org>
> Subject: Re: [VOTE] Requiring final keyword on every parameter and
> local variable
> Hi Jake,
>
> I agree with everyone's point about final being a useful, I just don't
> find it useful enough to do anything manually across the code base at this
> point.
>
> I believe first in foremost in no code warnings. Intellij warns about
> variables that can be final, so I make them final as it finds them. It is
> very rare that I am writing new code at this point. I have spent the last
> year bug fixing other people's code. From my standpoint, original intent
> is largely moot. So, for me, the question is do I go through code that is
> not mine and mark it all as final (where appropriate)? Sure, in code that I
> touch. In code the rest of the codebase, I think there are bigger fish to
> fry before we get to final.
>
> It seems like the larger portion of the consensus is to recommend that
> variables are marked final (where appropriate) as we find them or create
> them.
> That seems like the going forward approach consensus.
>
> I will be blunt though. This seems nitpicky *compared* to the number
> of code warnings there are and the fact that people are not actively fixing
> all of the warnings.
> If we don't come to the same consensus about all warnings this seems
> like painting a rusted car, sure it makes it all look the same, but does
> very little for the underlying problems. Now to undermine my own argument a
> little, I think that especially in release blockers, we want to touch as
> little code as possible, so I am flexible there.
>
> I would also like to agree with Udo about final really not being very
> good compared to Mutable and Immutable objects in other languages.
>
> Thanks,
> Mark
>
>
> On 4/15/21, 7:49 PM, "Udo Kohlmeyer" <ud...@vmware.com> 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?
>
> --Udo
>
> On 4/15/21, 1:19 PM, "Jacob Barrett" <ja...@vmware.com> wrote:
>
>
>
> > On Apr 14, 2021, at 7:46 PM, Udo Kohlmeyer <ud...@vmware.com>
> 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
Posted by John Blum <jb...@vmware.com>.
I vote -1 for (generally) using final within constructor/method parameters and local variables.
While final (e.g. on instance variables) has guaranteed benefits during construction (e.g. initialization safety of an object used in a muti-Threaded context) as Kirk rightly pointed out, its non-apparent, intended use can be ill-conceived and ill-received outside this context.
As Jason also mentioned, which cannot be overemphasized enough, final (by itself) does not guarantee immutability, which is often the intention of using final in the first place.
While using final ensures immutability of primitive types (e.g. int, & their corresponding wrapper types, i.e. Integer, only because the Integer class is immutable to begin with) it does not ensure immutability of reference types (immutable objects), arrays or Collection types. You can still change primitive arrays or arrays/Collections containing reference types, even when the array/Collection reference is final, regardless of whether an array/Collection is passed to a constructor or method. Additionally, arrays and Collections may contain mutable objects.
While there is some utility in using final, there are often much better techniques that can be applied, such as, but not limited to:
1) Strict validation and read-only use of arguments (effectively immutable).
2) Making defensive copies of arguments passed in where appropriate.
3) Limiting the scope of the (defined) variable. E.g. Using (private) utility methods to limit the scope and use (processing) of the variable passed into a constructor or method as an argument.
Keep in mind that Java is Pass by Value, not Pass by Reference, and in some cases, you can use this fact to your advantage.
So, because final is not a complete solution, this is why I vote -1.
NOTE: This does not mean I don't support using final in some cases (even for constructor or method parameters and even local variables), just that it should be used judiciously, like everything else, to explicitly express intent. And, most importantly, function should match intent.
-j
________________________________
From: Owen Nichols <on...@vmware.com>
Sent: Wednesday, April 21, 2021 5:16 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: [VOTE] Requiring final keyword on every parameter and local variable
Good point. We're probably stuck on vanilla JDK8 for years more anyway.
Since this discussion seems to be mainly about personal viewing preference, I wonder if someone has written an IntellJ plugin that would simply render effectively-final variables as if they were preceded by the word "final" (for people that want to see finals everywhere), or a plugin to collapse unnecessary finals (for people that don't want to see them).
On 4/21/21, 4:36 PM, "John Blum" <jb...@vmware.com> wrote:
I wouldn't recommend Lombok for production code, primarily because it generates code. Generated code occasionally leads to unintended consequences or incompatibilities with newer Java versions. Additionally, it requires as 3rd-party dependency to gain these capabilities rather than being part of the Java language itself. I'd rather Java include appropriate features for these 2 concerns, i.e. val & var, much like Kotlin, to more succinctly express intent. Even though Java now has var, it is different than Kotlin's var.
I only use Lombok for testing and demoing purposes.
-j
________________________________
From: Kirk Lund <kl...@apache.org>
Sent: Wednesday, April 21, 2021 12:59 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: [VOTE] Requiring final keyword on every parameter and local variable
I'm interested in https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprojectlombok.org%2F&data=04%7C01%7Cjblum%40vmware.com%7C02abefcbe3d64280ff9e08d90523f719%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637546474297335201%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=c5TTu5bpWjDlwMAG%2BYpG7YZduYa1oepW%2FLJ%2FvcFMx%2Fg%3D&reserved=0 and I remember John Blum using
it on some projects. I would like to study it and do some perf testing on
it before supporting it for Geode though. In general, I don't like
generated code and it looks like at least some of the features involve
generated code -- that's the only potential downside for me.
-Kirk
On Mon, Apr 19, 2021 at 12:57 PM Owen Nichols <on...@vmware.com> wrote:
> Any interest in adopting https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprojectlombok.org%2F&data=04%7C01%7Cjblum%40vmware.com%7C02abefcbe3d64280ff9e08d90523f719%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637546474297345194%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=MPVD7jfKKmRUuanKFzysHfJJxEVY1%2FneQBUvcLbIN38%3D&reserved=0 in Geode? Lombok
> allow a more readable "val" vs "var" syntax instead of "final" vs "" which
> could make it easier to do the right thing without "increasing visual
> noise".
>
> On 4/19/21, 12:40 PM, "Dale Emery" <de...@vmware.com> wrote:
>
> My test for whether enforce a guideline in a PR review is: Would I be
> willing to automate enforcing it in CI?
>
> I am -0 on this particular guideline.
>
> IntelliJ offers two competing inspections for Java coding style:
>
> * Local variable or parameter can be final
> * Unnecessary 'final' on local variable or parameter
>
> Both of these are (I think) disabled by default in IntelliJ. And
> neither satisfies my “I’d be willing to automate enforcing it” test.
>
> Dale
>
> From: Mark Hanson <ha...@vmware.com>
> Date: Monday, April 19, 2021 at 11:08 AM
> To: dev@geode.apache.org <de...@geode.apache.org>
> Subject: Re: [VOTE] Requiring final keyword on every parameter and
> local variable
> Hi Jake,
>
> I agree with everyone's point about final being a useful, I just don't
> find it useful enough to do anything manually across the code base at this
> point.
>
> I believe first in foremost in no code warnings. Intellij warns about
> variables that can be final, so I make them final as it finds them. It is
> very rare that I am writing new code at this point. I have spent the last
> year bug fixing other people's code. From my standpoint, original intent
> is largely moot. So, for me, the question is do I go through code that is
> not mine and mark it all as final (where appropriate)? Sure, in code that I
> touch. In code the rest of the codebase, I think there are bigger fish to
> fry before we get to final.
>
> It seems like the larger portion of the consensus is to recommend that
> variables are marked final (where appropriate) as we find them or create
> them.
> That seems like the going forward approach consensus.
>
> I will be blunt though. This seems nitpicky *compared* to the number
> of code warnings there are and the fact that people are not actively fixing
> all of the warnings.
> If we don't come to the same consensus about all warnings this seems
> like painting a rusted car, sure it makes it all look the same, but does
> very little for the underlying problems. Now to undermine my own argument a
> little, I think that especially in release blockers, we want to touch as
> little code as possible, so I am flexible there.
>
> I would also like to agree with Udo about final really not being very
> good compared to Mutable and Immutable objects in other languages.
>
> Thanks,
> Mark
>
>
> On 4/15/21, 7:49 PM, "Udo Kohlmeyer" <ud...@vmware.com> 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?
>
> --Udo
>
> On 4/15/21, 1:19 PM, "Jacob Barrett" <ja...@vmware.com> wrote:
>
>
>
> > On Apr 14, 2021, at 7:46 PM, Udo Kohlmeyer <ud...@vmware.com>
> 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
Posted by Owen Nichols <on...@vmware.com>.
Good point. We're probably stuck on vanilla JDK8 for years more anyway.
Since this discussion seems to be mainly about personal viewing preference, I wonder if someone has written an IntellJ plugin that would simply render effectively-final variables as if they were preceded by the word "final" (for people that want to see finals everywhere), or a plugin to collapse unnecessary finals (for people that don't want to see them).
On 4/21/21, 4:36 PM, "John Blum" <jb...@vmware.com> wrote:
I wouldn't recommend Lombok for production code, primarily because it generates code. Generated code occasionally leads to unintended consequences or incompatibilities with newer Java versions. Additionally, it requires as 3rd-party dependency to gain these capabilities rather than being part of the Java language itself. I'd rather Java include appropriate features for these 2 concerns, i.e. val & var, much like Kotlin, to more succinctly express intent. Even though Java now has var, it is different than Kotlin's var.
I only use Lombok for testing and demoing purposes.
-j
________________________________
From: Kirk Lund <kl...@apache.org>
Sent: Wednesday, April 21, 2021 12:59 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: [VOTE] Requiring final keyword on every parameter and local variable
I'm interested in https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprojectlombok.org%2F&data=04%7C01%7Conichols%40vmware.com%7C12ad0c3d5634430f31f808d9051e4d78%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637546449987523132%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=CDaPpdPe819GBlhtLa00DNXSi7HC%2Bm0weDUq2oHyGuo%3D&reserved=0 and I remember John Blum using
it on some projects. I would like to study it and do some perf testing on
it before supporting it for Geode though. In general, I don't like
generated code and it looks like at least some of the features involve
generated code -- that's the only potential downside for me.
-Kirk
On Mon, Apr 19, 2021 at 12:57 PM Owen Nichols <on...@vmware.com> wrote:
> Any interest in adopting https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprojectlombok.org%2F&data=04%7C01%7Conichols%40vmware.com%7C12ad0c3d5634430f31f808d9051e4d78%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637546449987523132%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=CDaPpdPe819GBlhtLa00DNXSi7HC%2Bm0weDUq2oHyGuo%3D&reserved=0 in Geode? Lombok
> allow a more readable "val" vs "var" syntax instead of "final" vs "" which
> could make it easier to do the right thing without "increasing visual
> noise".
>
> On 4/19/21, 12:40 PM, "Dale Emery" <de...@vmware.com> wrote:
>
> My test for whether enforce a guideline in a PR review is: Would I be
> willing to automate enforcing it in CI?
>
> I am -0 on this particular guideline.
>
> IntelliJ offers two competing inspections for Java coding style:
>
> * Local variable or parameter can be final
> * Unnecessary 'final' on local variable or parameter
>
> Both of these are (I think) disabled by default in IntelliJ. And
> neither satisfies my “I’d be willing to automate enforcing it” test.
>
> Dale
>
> From: Mark Hanson <ha...@vmware.com>
> Date: Monday, April 19, 2021 at 11:08 AM
> To: dev@geode.apache.org <de...@geode.apache.org>
> Subject: Re: [VOTE] Requiring final keyword on every parameter and
> local variable
> Hi Jake,
>
> I agree with everyone's point about final being a useful, I just don't
> find it useful enough to do anything manually across the code base at this
> point.
>
> I believe first in foremost in no code warnings. Intellij warns about
> variables that can be final, so I make them final as it finds them. It is
> very rare that I am writing new code at this point. I have spent the last
> year bug fixing other people's code. From my standpoint, original intent
> is largely moot. So, for me, the question is do I go through code that is
> not mine and mark it all as final (where appropriate)? Sure, in code that I
> touch. In code the rest of the codebase, I think there are bigger fish to
> fry before we get to final.
>
> It seems like the larger portion of the consensus is to recommend that
> variables are marked final (where appropriate) as we find them or create
> them.
> That seems like the going forward approach consensus.
>
> I will be blunt though. This seems nitpicky *compared* to the number
> of code warnings there are and the fact that people are not actively fixing
> all of the warnings.
> If we don't come to the same consensus about all warnings this seems
> like painting a rusted car, sure it makes it all look the same, but does
> very little for the underlying problems. Now to undermine my own argument a
> little, I think that especially in release blockers, we want to touch as
> little code as possible, so I am flexible there.
>
> I would also like to agree with Udo about final really not being very
> good compared to Mutable and Immutable objects in other languages.
>
> Thanks,
> Mark
>
>
> On 4/15/21, 7:49 PM, "Udo Kohlmeyer" <ud...@vmware.com> 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?
>
> --Udo
>
> On 4/15/21, 1:19 PM, "Jacob Barrett" <ja...@vmware.com> wrote:
>
>
>
> > On Apr 14, 2021, at 7:46 PM, Udo Kohlmeyer <ud...@vmware.com>
> 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
Posted by John Blum <jb...@vmware.com>.
I wouldn't recommend Lombok for production code, primarily because it generates code. Generated code occasionally leads to unintended consequences or incompatibilities with newer Java versions. Additionally, it requires as 3rd-party dependency to gain these capabilities rather than being part of the Java language itself. I'd rather Java include appropriate features for these 2 concerns, i.e. val & var, much like Kotlin, to more succinctly express intent. Even though Java now has var, it is different than Kotlin's var.
I only use Lombok for testing and demoing purposes.
-j
________________________________
From: Kirk Lund <kl...@apache.org>
Sent: Wednesday, April 21, 2021 12:59 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: [VOTE] Requiring final keyword on every parameter and local variable
I'm interested in https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprojectlombok.org%2F&data=04%7C01%7Cjblum%40vmware.com%7Cebf313cf7061468b6e8508d904fff633%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637546319662043634%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=yHceAr%2FbQtVTQAmSWxkNYkctPFP8nG6fph0QWLCRaDA%3D&reserved=0 and I remember John Blum using
it on some projects. I would like to study it and do some perf testing on
it before supporting it for Geode though. In general, I don't like
generated code and it looks like at least some of the features involve
generated code -- that's the only potential downside for me.
-Kirk
On Mon, Apr 19, 2021 at 12:57 PM Owen Nichols <on...@vmware.com> wrote:
> Any interest in adopting https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprojectlombok.org%2F&data=04%7C01%7Cjblum%40vmware.com%7Cebf313cf7061468b6e8508d904fff633%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637546319662043634%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=yHceAr%2FbQtVTQAmSWxkNYkctPFP8nG6fph0QWLCRaDA%3D&reserved=0 in Geode? Lombok
> allow a more readable "val" vs "var" syntax instead of "final" vs "" which
> could make it easier to do the right thing without "increasing visual
> noise".
>
> On 4/19/21, 12:40 PM, "Dale Emery" <de...@vmware.com> wrote:
>
> My test for whether enforce a guideline in a PR review is: Would I be
> willing to automate enforcing it in CI?
>
> I am -0 on this particular guideline.
>
> IntelliJ offers two competing inspections for Java coding style:
>
> * Local variable or parameter can be final
> * Unnecessary 'final' on local variable or parameter
>
> Both of these are (I think) disabled by default in IntelliJ. And
> neither satisfies my “I’d be willing to automate enforcing it” test.
>
> Dale
>
> From: Mark Hanson <ha...@vmware.com>
> Date: Monday, April 19, 2021 at 11:08 AM
> To: dev@geode.apache.org <de...@geode.apache.org>
> Subject: Re: [VOTE] Requiring final keyword on every parameter and
> local variable
> Hi Jake,
>
> I agree with everyone's point about final being a useful, I just don't
> find it useful enough to do anything manually across the code base at this
> point.
>
> I believe first in foremost in no code warnings. Intellij warns about
> variables that can be final, so I make them final as it finds them. It is
> very rare that I am writing new code at this point. I have spent the last
> year bug fixing other people's code. From my standpoint, original intent
> is largely moot. So, for me, the question is do I go through code that is
> not mine and mark it all as final (where appropriate)? Sure, in code that I
> touch. In code the rest of the codebase, I think there are bigger fish to
> fry before we get to final.
>
> It seems like the larger portion of the consensus is to recommend that
> variables are marked final (where appropriate) as we find them or create
> them.
> That seems like the going forward approach consensus.
>
> I will be blunt though. This seems nitpicky *compared* to the number
> of code warnings there are and the fact that people are not actively fixing
> all of the warnings.
> If we don't come to the same consensus about all warnings this seems
> like painting a rusted car, sure it makes it all look the same, but does
> very little for the underlying problems. Now to undermine my own argument a
> little, I think that especially in release blockers, we want to touch as
> little code as possible, so I am flexible there.
>
> I would also like to agree with Udo about final really not being very
> good compared to Mutable and Immutable objects in other languages.
>
> Thanks,
> Mark
>
>
> On 4/15/21, 7:49 PM, "Udo Kohlmeyer" <ud...@vmware.com> 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?
>
> --Udo
>
> On 4/15/21, 1:19 PM, "Jacob Barrett" <ja...@vmware.com> wrote:
>
>
>
> > On Apr 14, 2021, at 7:46 PM, Udo Kohlmeyer <ud...@vmware.com>
> 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
Posted by Kirk Lund <kl...@apache.org>.
I'm interested in https://projectlombok.org and I remember John Blum using
it on some projects. I would like to study it and do some perf testing on
it before supporting it for Geode though. In general, I don't like
generated code and it looks like at least some of the features involve
generated code -- that's the only potential downside for me.
-Kirk
On Mon, Apr 19, 2021 at 12:57 PM Owen Nichols <on...@vmware.com> wrote:
> Any interest in adopting https://projectlombok.org in Geode? Lombok
> allow a more readable "val" vs "var" syntax instead of "final" vs "" which
> could make it easier to do the right thing without "increasing visual
> noise".
>
> On 4/19/21, 12:40 PM, "Dale Emery" <de...@vmware.com> wrote:
>
> My test for whether enforce a guideline in a PR review is: Would I be
> willing to automate enforcing it in CI?
>
> I am -0 on this particular guideline.
>
> IntelliJ offers two competing inspections for Java coding style:
>
> * Local variable or parameter can be final
> * Unnecessary 'final' on local variable or parameter
>
> Both of these are (I think) disabled by default in IntelliJ. And
> neither satisfies my “I’d be willing to automate enforcing it” test.
>
> Dale
>
> From: Mark Hanson <ha...@vmware.com>
> Date: Monday, April 19, 2021 at 11:08 AM
> To: dev@geode.apache.org <de...@geode.apache.org>
> Subject: Re: [VOTE] Requiring final keyword on every parameter and
> local variable
> Hi Jake,
>
> I agree with everyone's point about final being a useful, I just don't
> find it useful enough to do anything manually across the code base at this
> point.
>
> I believe first in foremost in no code warnings. Intellij warns about
> variables that can be final, so I make them final as it finds them. It is
> very rare that I am writing new code at this point. I have spent the last
> year bug fixing other people's code. From my standpoint, original intent
> is largely moot. So, for me, the question is do I go through code that is
> not mine and mark it all as final (where appropriate)? Sure, in code that I
> touch. In code the rest of the codebase, I think there are bigger fish to
> fry before we get to final.
>
> It seems like the larger portion of the consensus is to recommend that
> variables are marked final (where appropriate) as we find them or create
> them.
> That seems like the going forward approach consensus.
>
> I will be blunt though. This seems nitpicky *compared* to the number
> of code warnings there are and the fact that people are not actively fixing
> all of the warnings.
> If we don't come to the same consensus about all warnings this seems
> like painting a rusted car, sure it makes it all look the same, but does
> very little for the underlying problems. Now to undermine my own argument a
> little, I think that especially in release blockers, we want to touch as
> little code as possible, so I am flexible there.
>
> I would also like to agree with Udo about final really not being very
> good compared to Mutable and Immutable objects in other languages.
>
> Thanks,
> Mark
>
>
> On 4/15/21, 7:49 PM, "Udo Kohlmeyer" <ud...@vmware.com> 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?
>
> --Udo
>
> On 4/15/21, 1:19 PM, "Jacob Barrett" <ja...@vmware.com> wrote:
>
>
>
> > On Apr 14, 2021, at 7:46 PM, Udo Kohlmeyer <ud...@vmware.com>
> 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
Posted by Owen Nichols <on...@vmware.com>.
Any interest in adopting https://projectlombok.org in Geode? Lombok allow a more readable "val" vs "var" syntax instead of "final" vs "" which could make it easier to do the right thing without "increasing visual noise".
On 4/19/21, 12:40 PM, "Dale Emery" <de...@vmware.com> wrote:
My test for whether enforce a guideline in a PR review is: Would I be willing to automate enforcing it in CI?
I am -0 on this particular guideline.
IntelliJ offers two competing inspections for Java coding style:
* Local variable or parameter can be final
* Unnecessary 'final' on local variable or parameter
Both of these are (I think) disabled by default in IntelliJ. And neither satisfies my “I’d be willing to automate enforcing it” test.
Dale
From: Mark Hanson <ha...@vmware.com>
Date: Monday, April 19, 2021 at 11:08 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: [VOTE] Requiring final keyword on every parameter and local variable
Hi Jake,
I agree with everyone's point about final being a useful, I just don't find it useful enough to do anything manually across the code base at this point.
I believe first in foremost in no code warnings. Intellij warns about variables that can be final, so I make them final as it finds them. It is very rare that I am writing new code at this point. I have spent the last year bug fixing other people's code. From my standpoint, original intent is largely moot. So, for me, the question is do I go through code that is not mine and mark it all as final (where appropriate)? Sure, in code that I touch. In code the rest of the codebase, I think there are bigger fish to fry before we get to final.
It seems like the larger portion of the consensus is to recommend that variables are marked final (where appropriate) as we find them or create them.
That seems like the going forward approach consensus.
I will be blunt though. This seems nitpicky *compared* to the number of code warnings there are and the fact that people are not actively fixing all of the warnings.
If we don't come to the same consensus about all warnings this seems like painting a rusted car, sure it makes it all look the same, but does very little for the underlying problems. Now to undermine my own argument a little, I think that especially in release blockers, we want to touch as little code as possible, so I am flexible there.
I would also like to agree with Udo about final really not being very good compared to Mutable and Immutable objects in other languages.
Thanks,
Mark
On 4/15/21, 7:49 PM, "Udo Kohlmeyer" <ud...@vmware.com> 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?
--Udo
On 4/15/21, 1:19 PM, "Jacob Barrett" <ja...@vmware.com> wrote:
> On Apr 14, 2021, at 7:46 PM, Udo Kohlmeyer <ud...@vmware.com> 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
Posted by Dale Emery <de...@vmware.com>.
My test for whether enforce a guideline in a PR review is: Would I be willing to automate enforcing it in CI?
I am -0 on this particular guideline.
IntelliJ offers two competing inspections for Java coding style:
* Local variable or parameter can be final
* Unnecessary 'final' on local variable or parameter
Both of these are (I think) disabled by default in IntelliJ. And neither satisfies my “I’d be willing to automate enforcing it” test.
Dale
From: Mark Hanson <ha...@vmware.com>
Date: Monday, April 19, 2021 at 11:08 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: [VOTE] Requiring final keyword on every parameter and local variable
Hi Jake,
I agree with everyone's point about final being a useful, I just don't find it useful enough to do anything manually across the code base at this point.
I believe first in foremost in no code warnings. Intellij warns about variables that can be final, so I make them final as it finds them. It is very rare that I am writing new code at this point. I have spent the last year bug fixing other people's code. From my standpoint, original intent is largely moot. So, for me, the question is do I go through code that is not mine and mark it all as final (where appropriate)? Sure, in code that I touch. In code the rest of the codebase, I think there are bigger fish to fry before we get to final.
It seems like the larger portion of the consensus is to recommend that variables are marked final (where appropriate) as we find them or create them.
That seems like the going forward approach consensus.
I will be blunt though. This seems nitpicky *compared* to the number of code warnings there are and the fact that people are not actively fixing all of the warnings.
If we don't come to the same consensus about all warnings this seems like painting a rusted car, sure it makes it all look the same, but does very little for the underlying problems. Now to undermine my own argument a little, I think that especially in release blockers, we want to touch as little code as possible, so I am flexible there.
I would also like to agree with Udo about final really not being very good compared to Mutable and Immutable objects in other languages.
Thanks,
Mark
On 4/15/21, 7:49 PM, "Udo Kohlmeyer" <ud...@vmware.com> 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?
--Udo
On 4/15/21, 1:19 PM, "Jacob Barrett" <ja...@vmware.com> wrote:
> On Apr 14, 2021, at 7:46 PM, Udo Kohlmeyer <ud...@vmware.com> 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
Posted by Mark Hanson <ha...@vmware.com>.
Hi Jake,
I agree with everyone's point about final being a useful, I just don't find it useful enough to do anything manually across the code base at this point.
I believe first in foremost in no code warnings. Intellij warns about variables that can be final, so I make them final as it finds them. It is very rare that I am writing new code at this point. I have spent the last year bug fixing other people's code. From my standpoint, original intent is largely moot. So, for me, the question is do I go through code that is not mine and mark it all as final (where appropriate)? Sure, in code that I touch. In code the rest of the codebase, I think there are bigger fish to fry before we get to final.
It seems like the larger portion of the consensus is to recommend that variables are marked final (where appropriate) as we find them or create them.
That seems like the going forward approach consensus.
I will be blunt though. This seems nitpicky *compared* to the number of code warnings there are and the fact that people are not actively fixing all of the warnings.
If we don't come to the same consensus about all warnings this seems like painting a rusted car, sure it makes it all look the same, but does very little for the underlying problems. Now to undermine my own argument a little, I think that especially in release blockers, we want to touch as little code as possible, so I am flexible there.
I would also like to agree with Udo about final really not being very good compared to Mutable and Immutable objects in other languages.
Thanks,
Mark
On 4/15/21, 7:49 PM, "Udo Kohlmeyer" <ud...@vmware.com> 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?
--Udo
On 4/15/21, 1:19 PM, "Jacob Barrett" <ja...@vmware.com> wrote:
> On Apr 14, 2021, at 7:46 PM, Udo Kohlmeyer <ud...@vmware.com> 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
Posted by Jacob Barrett <ja...@vmware.com>.
> On Apr 15, 2021, at 7:49 PM, Udo Kohlmeyer <ud...@vmware.com> 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
Re: [VOTE] Requiring final keyword on every parameter and local
variable
Posted by Udo Kohlmeyer <ud...@vmware.com>.
@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" <ja...@vmware.com> wrote:
> On Apr 14, 2021, at 7:46 PM, Udo Kohlmeyer <ud...@vmware.com> 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
Posted by Jacob Barrett <ja...@vmware.com>.
> On Apr 14, 2021, at 7:46 PM, Udo Kohlmeyer <ud...@vmware.com> 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
Posted by Kirk Lund <kl...@apache.org>.
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 <br...@vmware.com> 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" <ud...@vmware.com> 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" <ja...@vmware.com> 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 <kl...@apache.org>
> 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
Posted by Bruce Schuchardt <br...@vmware.com>.
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" <ud...@vmware.com> 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" <ja...@vmware.com> 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 <kl...@apache.org> 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
Posted by Udo Kohlmeyer <ud...@vmware.com>.
"+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" <ja...@vmware.com> 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 <kl...@apache.org> 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
Posted by Jacob Barrett <ja...@vmware.com>.
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 <kl...@apache.org> 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
Posted by Kirk Lund <kl...@apache.org>.
I'm planning to leave this VOTE and DISCUSSION open until Wednesday 2400 PM
UTC.
On Wed, Apr 14, 2021 at 12:55 PM Kirk Lund <kl...@apache.org> 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
>
>