You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@flink.apache.org by Zili Chen <wa...@gmail.com> on 2019/10/01 19:44:09 UTC

[CODE STYLE] Parameters of method are always final

Hi devs,

Coming from this discussion[1] I'd like to propose that in Flink codebase
we suggest a code style
that parameters of method are always final. Almost everywhere parameters of
method are final
already and if we have such consensus we can prevent redundant final
modifier in method
declaration so that we survive from those noise.

Here are some cases that might require to modify a parameter.

1. to set default; especially if (param == null) { param = ... }
2. to refine parameter; it is in pattern if ( ... ) { param =
refine(param); }

Either of the cases we can move the refine/set default logics to the caller
or select another
name for the refined value case by case.

Looking forward to your feedbacks :-)

Best,
tison.

[1] https://github.com/apache/flink/pull/9788#discussion_r329314681

Re: [CODE STYLE] Parameters of method are always final

Posted by Arvid Heise <ar...@ververica.com>.

For Piotr's comment. IntelliJ does support adding/removing final parameters
automatically.

You could even automatically add them on save with the Save Actions plugin
[1].

[1] https://plugins.jetbrains.com/plugin/7642-save-actions

On Mon, Oct 7, 2019 at 11:22 AM Piotr Nowojski <pi...@ververica.com> wrote:

> After checking out the check style modules mentioned by Tison, I really do
> not see a point of enforcing/adding `final`. With ParameterAssignment [1]
> it’s redundant and will cause problems that I mentioned before.
>
> Additionally enabling ParameterAssignment [1] would be probably much
> easier in our code base compared to enabling FinalParameters [2].
>
> However still I’m not sure if that’s worth our troubles. I would be in
> favour of doing it only If enabling ParameterAssignment [1] doesn’t require
> many changes in our code base.
>
> Piotrek
>
> [1]
> https://checkstyle.sourceforge.io/config_coding.html#ParameterAssignment <
> https://checkstyle.sourceforge.io/config_coding.html#ParameterAssignment>
> [2] https://checkstyle.sourceforge.io/config_misc.html#FinalParameters <
> https://checkstyle.sourceforge.io/config_misc.html#FinalParameters>
>
> > On 7 Oct 2019, at 11:09, Dawid Wysakowicz <dw...@apache.org>
> wrote:
> >
> > Hi all,
> >
> > My quick take on it.
> >
> > 1. If I were to introduce any rule on that I agree with Aljoscha, we
> > should rather enforce the `final` keyword rather than the opposite.
> >
> > 2. I think it does not make sense to enforce rules on such an
> > unimportant (in my opinion) topic. Generally I agree with Piotr that if
> > we want to introduce some rule we should have a way to automatically
> > enforce it.
> >
> > 3. I also agree with Piotr that problems with parameters reassignment
> > appear nearly exclusively in a very long methods. If we break long
> > methods in a shorter ones than there is usually no problem with a
> > parameter reassignment.
> >
> > 4. I would be ok with adding a point to our code style guidelines, but
> > honestly I am a bit skeptical. In the end we are not writing a book on
> > how to write a good software, but we rather list the most important
> > problems that we've seen so far in Flink code base and how to better
> > solve it. I'm not sure that's the case with the parameters reassignment.
> >
> > Best,
> >
> > Dawid
> >
> > On 07/10/2019 10:11, Zili Chen wrote:
> >> Thanks for your thoughts Arvid & Piotr,
> >>
> >> I check out the effect of ParameterAssignment[1] and it does
> >> prevent codes from modifying parameter which I argued above
> >> the most value introduced by `final` modifier of parameter.
> >>
> >> So firstly, I think it's good to enable ParameterAssignment in our
> >> codebase.
> >>
> >> Besides, there is no rule to forbid `final` modifier of parameter.
> >> Instead, there is a rule to enforce `final` modifier[2] but given [1]
> >> enabled it is actually redundant.
> >>
> >> The main purpose for, given enforced not modify parameters, reducing
> >> `final` modifiers of parameter is to remove redundant modifier so that
> we
> >> don't see have declaration like
> >>
> >> T fn(
> >>  final ArgumentType1 argument1,
> >>  final ArgumentType2 argument2,
> >>  ...)
> >>
> >> because we actually don't mark final everywhere so it might make some
> >> confusions.
> >>
> >> Given [1] is enforced these `final` are indeed redundant so that we can
> >> add a convention to reduce `final` modifiers of parameters, which is a
> net
> >> win.
> >>
> >> Best,
> >> tison.
> >>
> >> [1]
> https://checkstyle.sourceforge.io/config_coding.html#ParameterAssignment
> >> [2] https://checkstyle.sourceforge.io/config_misc.html#FinalParameters
> >>
> >>
> >>
> >> Piotr Nowojski <pi...@ververica.com> 于2019年10月7日周一 下午3:49写道：
> >>
> >>> Hi,
> >>>
> >>> Couple of arguments to counter the proposal of making the `final`
> keyword
> >>> obligatory. Can we prepare a code style/IDE settings to add it
> >>> automatically? If not, I would be strongly against it, since:
> >>>
> >>> - Intellij’s automatic refactor actions will not work properly.
> >>> - I don’t think it’s a big deal. I don’t remember having any issues
> with
> >>> the lack or presence of the `final` keyword.
> >>> - `final` is pretty much useless in most of the cases (it’s not `const`
> >>> and it works only for the primitive types).
> >>> - I don’t like the extra overhead of doing something of very little
> extra
> >>> value. Especially the extra hassle of going back & forth during the
> reviews
> >>> (both as a contributor & as a reviewer).
> >>> - If missing `final` keyword caused some confusion, because
> surprisingly a
> >>> parameter was modified somewhere in the function and it wasn’t
> obviously
> >>> visible, the function is doing probably too many things and it’s too
> >>> long/too complicated…
> >>>
> >>> Generally speaking, I’m against adding minor things to our codestyle
> that
> >>> can not be enforced and added automatically.
> >>>
> >>> Piotrek
> >>>
> >>>> On 7 Oct 2019, at 09:13, Arvid Heise <ar...@ververica.com> wrote:
> >>>>
> >>>> Hi guys,
> >>>>
> >>>> I'm a bit torn. In general, +1 for making parameters effectively
> final.
> >>>>
> >>>> The question is how to enforce it. We can make it explicit (like
> Aljoscha
> >>>> said). All IDEs will show immediately warnings/errors for violations.
> It
> >>>> would allow to softly migrate code.
> >>>>
> >>>> Another option is to use a checkstyle rule [1]. Then, we could omit
> the
> >>>> final keyword and rely on checkstyle checks as we do for quite a few
> >>> other
> >>>> things. A hard checkstyle rule would probably fail on a good portion
> of
> >>> the
> >>>> current code base. But we would also remove reassignment to parameters
> >>>> (which I consider an anti-pattern).
> >>>>
> >>>> If we opt not to enforce it, then -1 for removing final keywords from
> my
> >>>> side.
> >>>>
> >>>> Best,
> >>>>
> >>>> Arvid
> >>>>
> >>>> [1]
> >>>>
> >>>
> https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/checks/coding/ParameterAssignmentCheck.html
> >>>> On Fri, Oct 4, 2019 at 1:22 PM Zili Chen <wa...@gmail.com>
> wrote:
> >>>>
> >>>>> Hi Aljoscha,
> >>>>>
> >>>>> I totally agree with you on field topic. Of course it makes
> significant
> >>>>> difference whether
> >>>>> or not a field is final and IDE/compiler can help on checking.
> >>>>>
> >>>>> Here are several thoughts about final modifier on parameters and why
> I
> >>>>> propose this one:
> >>>>>
> >>>>> 1. parameters should be always final
> >>>>>
> >>>>> As described above, there is no reason a parameter to be non-final.
> So
> >>>>> different with field,
> >>>>> a field can be final or non-final based on whether or not it is
> >>> immutable.
> >>>>> Thus with such a
> >>>>> code style guide in our community, we can work towards a codebase
> every
> >>>>> parameter is
> >>>>> effectively final.
> >>>>>
> >>>>> 2. parameter final cannot be inherited
> >>>>>
> >>>>> Unfortunately Java doesn't keep final modifier of method parameter
> when
> >>>>> inheritance. So
> >>>>> even you mark a parameter as final in an interface or super class,
> you
> >>> have
> >>>>> to re-mark it
> >>>>> as final in its implementation or subclass. From another perspective,
> >>> final
> >>>>> modifier of
> >>>>> parameter is a local attribute of parameter so that we can narrow
> >>> possible
> >>>>> effect during
> >>>>> review.
> >>>>>
> >>>>> 3. IDE can lint difference between effective final and mutable
> parameter
> >>>>>
> >>>>> It is true that IDE such as Intellij IDEA can lint difference between
> >>>>> effective final and
> >>>>> mutable parameter(with underline). So that with this code style what
> we
> >>>>> lose is that
> >>>>> we cannot get a compile time error if someone modifies parameter in
> the
> >>>>> method body.
> >>>>> But as mentioned in 1, by no means we allow a parameter to be
> modified.
> >>> If
> >>>>> we agree
> >>>>> on this statement, then we hopefully converge in a codebase that no
> >>>>> parameter is
> >>>>> modified.
> >>>>>
> >>>>> For what we gain, I'd like to recur our existing code style of
> @Nonnull
> >>> to
> >>>>> be default.
> >>>>> Actually it does help for compiler to report compile time warning if
> we
> >>>>> possibly pass a
> >>>>> nullable value to an non-null field. We make @Nonnull as default to
> >>> "reduce
> >>>>> code noise"
> >>>>> so I think we can reuse the statement here also.
> >>>>>
> >>>>> Best,
> >>>>> tison.
> >>>>>
> >>>>>
> >>>>> Aljoscha Krettek <al...@apache.org> 于2019年10月4日周五 下午5:58写道：
> >>>>>
> >>>>>> I actually think that doing this the other way round would be
> correct.
> >>>>>> Removing final everywhere and relying on humans to assume that
> >>> everything
> >>>>>> is final doesn’t seem maintainable to me. The benefit of having
> final
> >>> on
> >>>>>> parameters/fields is that the compiler/IDE actually checks that you
> >>> don’t
> >>>>>> modify it.
> >>>>>>
> >>>>>> In general, I think that as much as possible should be declared
> final,
> >>>>>> including fields and parameters.
> >>>>>>
> >>>>>> Best,
> >>>>>> Aljoscha
> >>>>>>
> >>>>>>> On 2. Oct 2019, at 13:31, Piotr Nowojski <pi...@ververica.com>
> wrote:
> >>>>>>>
> >>>>>>> +1 from my side.
> >>>>>>>
> >>>>>>>> On 2 Oct 2019, at 13:07, Zili Chen <wa...@gmail.com> wrote:
> >>>>>>>>
> >>>>>>>> Yes exactly.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Piotr Nowojski <pi...@ververica.com> 于2019年10月2日周三 下午7:03写道：
> >>>>>>>>
> >>>>>>>>> Hi Tison,
> >>>>>>>>>
> >>>>>>>>> To clarify      your proposal. You are proposing to actually drop
> >>> the
> >>>>>>>>> `final` keyword from the parameters and we should implicilty
> assume
> >>>>>> that
> >>>>>>>>> it’s always there (in other words, we shouldn’t be modifying the
> >>>>>>>>> parameters). Did I understand this correctly?
> >>>>>>>>>
> >>>>>>>>> Piotrek
> >>>>>>>>>
> >>>>>>>>>> On 1 Oct 2019, at 21:44, Zili Chen <wa...@gmail.com>
> wrote:
> >>>>>>>>>>
> >>>>>>>>>> Hi devs,
> >>>>>>>>>>
> >>>>>>>>>> Coming from this discussion[1] I'd like to propose that in Flink
> >>>>>> codebase
> >>>>>>>>>> we suggest a code style
> >>>>>>>>>> that parameters of method are always final. Almost everywhere
> >>>>>> parameters
> >>>>>>>>> of
> >>>>>>>>>> method are final
> >>>>>>>>>> already and if we have such consensus we can prevent redundant
> >>> final
> >>>>>>>>>> modifier in method
> >>>>>>>>>> declaration so that we survive from those noise.
> >>>>>>>>>>
> >>>>>>>>>> Here are some cases that might require to modify a parameter.
> >>>>>>>>>>
> >>>>>>>>>> 1. to set default; especially if (param == null) { param = ... }
> >>>>>>>>>> 2. to refine parameter; it is in pattern if ( ... ) { param =
> >>>>>>>>>> refine(param); }
> >>>>>>>>>>
> >>>>>>>>>> Either of the cases we can move the refine/set default logics to
> >>> the
> >>>>>>>>> caller
> >>>>>>>>>> or select another
> >>>>>>>>>> name for the refined value case by case.
> >>>>>>>>>>
> >>>>>>>>>> Looking forward to your feedbacks :-)
> >>>>>>>>>>
> >>>>>>>>>> Best,
> >>>>>>>>>> tison.
> >>>>>>>>>>
> >>>>>>>>>> [1]
> >>> https://github.com/apache/flink/pull/9788#discussion_r329314681
> >>>>>>>>>
> >>>>>>
> >>>
> >
>
>

Re: [CODE STYLE] Parameters of method are always final

Posted by Piotr Nowojski <pi...@ververica.com>.

After checking out the check style modules mentioned by Tison, I really do not see a point of enforcing/adding `final`. With ParameterAssignment [1] it’s redundant and will cause problems that I mentioned before.

Additionally enabling ParameterAssignment [1] would be probably much easier in our code base compared to enabling FinalParameters [2].

However still I’m not sure if that’s worth our troubles. I would be in favour of doing it only If enabling ParameterAssignment [1] doesn’t require many changes in our code base.

Piotrek

[1] https://checkstyle.sourceforge.io/config_coding.html#ParameterAssignment <https://checkstyle.sourceforge.io/config_coding.html#ParameterAssignment>
[2] https://checkstyle.sourceforge.io/config_misc.html#FinalParameters <https://checkstyle.sourceforge.io/config_misc.html#FinalParameters>

> On 7 Oct 2019, at 11:09, Dawid Wysakowicz <dw...@apache.org> wrote:
> 
> Hi all,
> 
> My quick take on it.
> 
> 1. If I were to introduce any rule on that I agree with Aljoscha, we
> should rather enforce the `final` keyword rather than the opposite.
> 
> 2. I think it does not make sense to enforce rules on such an
> unimportant (in my opinion) topic. Generally I agree with Piotr that if
> we want to introduce some rule we should have a way to automatically
> enforce it.
> 
> 3. I also agree with Piotr that problems with parameters reassignment
> appear nearly exclusively in a very long methods. If we break long
> methods in a shorter ones than there is usually no problem with a
> parameter reassignment.
> 
> 4. I would be ok with adding a point to our code style guidelines, but
> honestly I am a bit skeptical. In the end we are not writing a book on
> how to write a good software, but we rather list the most important
> problems that we've seen so far in Flink code base and how to better
> solve it. I'm not sure that's the case with the parameters reassignment.
> 
> Best,
> 
> Dawid
> 
> On 07/10/2019 10:11, Zili Chen wrote:
>> Thanks for your thoughts Arvid & Piotr,
>> 
>> I check out the effect of ParameterAssignment[1] and it does
>> prevent codes from modifying parameter which I argued above
>> the most value introduced by `final` modifier of parameter.
>> 
>> So firstly, I think it's good to enable ParameterAssignment in our
>> codebase.
>> 
>> Besides, there is no rule to forbid `final` modifier of parameter.
>> Instead, there is a rule to enforce `final` modifier[2] but given [1]
>> enabled it is actually redundant.
>> 
>> The main purpose for, given enforced not modify parameters, reducing
>> `final` modifiers of parameter is to remove redundant modifier so that we
>> don't see have declaration like
>> 
>> T fn(
>>  final ArgumentType1 argument1,
>>  final ArgumentType2 argument2,
>>  ...)
>> 
>> because we actually don't mark final everywhere so it might make some
>> confusions.
>> 
>> Given [1] is enforced these `final` are indeed redundant so that we can
>> add a convention to reduce `final` modifiers of parameters, which is a net
>> win.
>> 
>> Best,
>> tison.
>> 
>> [1] https://checkstyle.sourceforge.io/config_coding.html#ParameterAssignment
>> [2] https://checkstyle.sourceforge.io/config_misc.html#FinalParameters
>> 
>> 
>> 
>> Piotr Nowojski <pi...@ververica.com> 于2019年10月7日周一 下午3:49写道：
>> 
>>> Hi,
>>> 
>>> Couple of arguments to counter the proposal of making the `final` keyword
>>> obligatory. Can we prepare a code style/IDE settings to add it
>>> automatically? If not, I would be strongly against it, since:
>>> 
>>> - Intellij’s automatic refactor actions will not work properly.
>>> - I don’t think it’s a big deal. I don’t remember having any issues with
>>> the lack or presence of the `final` keyword.
>>> - `final` is pretty much useless in most of the cases (it’s not `const`
>>> and it works only for the primitive types).
>>> - I don’t like the extra overhead of doing something of very little extra
>>> value. Especially the extra hassle of going back & forth during the reviews
>>> (both as a contributor & as a reviewer).
>>> - If missing `final` keyword caused some confusion, because surprisingly a
>>> parameter was modified somewhere in the function and it wasn’t obviously
>>> visible, the function is doing probably too many things and it’s too
>>> long/too complicated…
>>> 
>>> Generally speaking, I’m against adding minor things to our codestyle that
>>> can not be enforced and added automatically.
>>> 
>>> Piotrek
>>> 
>>>> On 7 Oct 2019, at 09:13, Arvid Heise <ar...@ververica.com> wrote:
>>>> 
>>>> Hi guys,
>>>> 
>>>> I'm a bit torn. In general, +1 for making parameters effectively final.
>>>> 
>>>> The question is how to enforce it. We can make it explicit (like Aljoscha
>>>> said). All IDEs will show immediately warnings/errors for violations. It
>>>> would allow to softly migrate code.
>>>> 
>>>> Another option is to use a checkstyle rule [1]. Then, we could omit the
>>>> final keyword and rely on checkstyle checks as we do for quite a few
>>> other
>>>> things. A hard checkstyle rule would probably fail on a good portion of
>>> the
>>>> current code base. But we would also remove reassignment to parameters
>>>> (which I consider an anti-pattern).
>>>> 
>>>> If we opt not to enforce it, then -1 for removing final keywords from my
>>>> side.
>>>> 
>>>> Best,
>>>> 
>>>> Arvid
>>>> 
>>>> [1]
>>>> 
>>> https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/checks/coding/ParameterAssignmentCheck.html
>>>> On Fri, Oct 4, 2019 at 1:22 PM Zili Chen <wa...@gmail.com> wrote:
>>>> 
>>>>> Hi Aljoscha,
>>>>> 
>>>>> I totally agree with you on field topic. Of course it makes significant
>>>>> difference whether
>>>>> or not a field is final and IDE/compiler can help on checking.
>>>>> 
>>>>> Here are several thoughts about final modifier on parameters and why I
>>>>> propose this one:
>>>>> 
>>>>> 1. parameters should be always final
>>>>> 
>>>>> As described above, there is no reason a parameter to be non-final. So
>>>>> different with field,
>>>>> a field can be final or non-final based on whether or not it is
>>> immutable.
>>>>> Thus with such a
>>>>> code style guide in our community, we can work towards a codebase every
>>>>> parameter is
>>>>> effectively final.
>>>>> 
>>>>> 2. parameter final cannot be inherited
>>>>> 
>>>>> Unfortunately Java doesn't keep final modifier of method parameter when
>>>>> inheritance. So
>>>>> even you mark a parameter as final in an interface or super class, you
>>> have
>>>>> to re-mark it
>>>>> as final in its implementation or subclass. From another perspective,
>>> final
>>>>> modifier of
>>>>> parameter is a local attribute of parameter so that we can narrow
>>> possible
>>>>> effect during
>>>>> review.
>>>>> 
>>>>> 3. IDE can lint difference between effective final and mutable parameter
>>>>> 
>>>>> It is true that IDE such as Intellij IDEA can lint difference between
>>>>> effective final and
>>>>> mutable parameter(with underline). So that with this code style what we
>>>>> lose is that
>>>>> we cannot get a compile time error if someone modifies parameter in the
>>>>> method body.
>>>>> But as mentioned in 1, by no means we allow a parameter to be modified.
>>> If
>>>>> we agree
>>>>> on this statement, then we hopefully converge in a codebase that no
>>>>> parameter is
>>>>> modified.
>>>>> 
>>>>> For what we gain, I'd like to recur our existing code style of @Nonnull
>>> to
>>>>> be default.
>>>>> Actually it does help for compiler to report compile time warning if we
>>>>> possibly pass a
>>>>> nullable value to an non-null field. We make @Nonnull as default to
>>> "reduce
>>>>> code noise"
>>>>> so I think we can reuse the statement here also.
>>>>> 
>>>>> Best,
>>>>> tison.
>>>>> 
>>>>> 
>>>>> Aljoscha Krettek <al...@apache.org> 于2019年10月4日周五 下午5:58写道：
>>>>> 
>>>>>> I actually think that doing this the other way round would be correct.
>>>>>> Removing final everywhere and relying on humans to assume that
>>> everything
>>>>>> is final doesn’t seem maintainable to me. The benefit of having final
>>> on
>>>>>> parameters/fields is that the compiler/IDE actually checks that you
>>> don’t
>>>>>> modify it.
>>>>>> 
>>>>>> In general, I think that as much as possible should be declared final,
>>>>>> including fields and parameters.
>>>>>> 
>>>>>> Best,
>>>>>> Aljoscha
>>>>>> 
>>>>>>> On 2. Oct 2019, at 13:31, Piotr Nowojski <pi...@ververica.com> wrote:
>>>>>>> 
>>>>>>> +1 from my side.
>>>>>>> 
>>>>>>>> On 2 Oct 2019, at 13:07, Zili Chen <wa...@gmail.com> wrote:
>>>>>>>> 
>>>>>>>> Yes exactly.
>>>>>>>> 
>>>>>>>> 
>>>>>>>> Piotr Nowojski <pi...@ververica.com> 于2019年10月2日周三 下午7:03写道：
>>>>>>>> 
>>>>>>>>> Hi Tison,
>>>>>>>>> 
>>>>>>>>> To clarify      your proposal. You are proposing to actually drop
>>> the
>>>>>>>>> `final` keyword from the parameters and we should implicilty assume
>>>>>> that
>>>>>>>>> it’s always there (in other words, we shouldn’t be modifying the
>>>>>>>>> parameters). Did I understand this correctly?
>>>>>>>>> 
>>>>>>>>> Piotrek
>>>>>>>>> 
>>>>>>>>>> On 1 Oct 2019, at 21:44, Zili Chen <wa...@gmail.com> wrote:
>>>>>>>>>> 
>>>>>>>>>> Hi devs,
>>>>>>>>>> 
>>>>>>>>>> Coming from this discussion[1] I'd like to propose that in Flink
>>>>>> codebase
>>>>>>>>>> we suggest a code style
>>>>>>>>>> that parameters of method are always final. Almost everywhere
>>>>>> parameters
>>>>>>>>> of
>>>>>>>>>> method are final
>>>>>>>>>> already and if we have such consensus we can prevent redundant
>>> final
>>>>>>>>>> modifier in method
>>>>>>>>>> declaration so that we survive from those noise.
>>>>>>>>>> 
>>>>>>>>>> Here are some cases that might require to modify a parameter.
>>>>>>>>>> 
>>>>>>>>>> 1. to set default; especially if (param == null) { param = ... }
>>>>>>>>>> 2. to refine parameter; it is in pattern if ( ... ) { param =
>>>>>>>>>> refine(param); }
>>>>>>>>>> 
>>>>>>>>>> Either of the cases we can move the refine/set default logics to
>>> the
>>>>>>>>> caller
>>>>>>>>>> or select another
>>>>>>>>>> name for the refined value case by case.
>>>>>>>>>> 
>>>>>>>>>> Looking forward to your feedbacks :-)
>>>>>>>>>> 
>>>>>>>>>> Best,
>>>>>>>>>> tison.
>>>>>>>>>> 
>>>>>>>>>> [1]
>>> https://github.com/apache/flink/pull/9788#discussion_r329314681
>>>>>>>>> 
>>>>>> 
>>> 
>

Re: [CODE STYLE] Parameters of method are always final

Posted by Dawid Wysakowicz <dw...@apache.org>.

Hi all,

My quick take on it.

1. If I were to introduce any rule on that I agree with Aljoscha, we
should rather enforce the `final` keyword rather than the opposite.

2. I think it does not make sense to enforce rules on such an
unimportant (in my opinion) topic. Generally I agree with Piotr that if
we want to introduce some rule we should have a way to automatically
enforce it.

3. I also agree with Piotr that problems with parameters reassignment
appear nearly exclusively in a very long methods. If we break long
methods in a shorter ones than there is usually no problem with a
parameter reassignment.

4. I would be ok with adding a point to our code style guidelines, but
honestly I am a bit skeptical. In the end we are not writing a book on
how to write a good software, but we rather list the most important
problems that we've seen so far in Flink code base and how to better
solve it. I'm not sure that's the case with the parameters reassignment.

Best,

Dawid

On 07/10/2019 10:11, Zili Chen wrote:
> Thanks for your thoughts Arvid & Piotr,
>
> I check out the effect of ParameterAssignment[1] and it does
> prevent codes from modifying parameter which I argued above
> the most value introduced by `final` modifier of parameter.
>
> So firstly, I think it's good to enable ParameterAssignment in our
> codebase.
>
> Besides, there is no rule to forbid `final` modifier of parameter.
> Instead, there is a rule to enforce `final` modifier[2] but given [1]
> enabled it is actually redundant.
>
> The main purpose for, given enforced not modify parameters, reducing
> `final` modifiers of parameter is to remove redundant modifier so that we
> don't see have declaration like
>
> T fn(
>   final ArgumentType1 argument1,
>   final ArgumentType2 argument2,
>   ...)
>
> because we actually don't mark final everywhere so it might make some
> confusions.
>
> Given [1] is enforced these `final` are indeed redundant so that we can
> add a convention to reduce `final` modifiers of parameters, which is a net
> win.
>
> Best,
> tison.
>
> [1] https://checkstyle.sourceforge.io/config_coding.html#ParameterAssignment
> [2] https://checkstyle.sourceforge.io/config_misc.html#FinalParameters
>
>
>
> Piotr Nowojski <pi...@ververica.com> 于2019年10月7日周一 下午3:49写道：
>
>> Hi,
>>
>> Couple of arguments to counter the proposal of making the `final` keyword
>> obligatory. Can we prepare a code style/IDE settings to add it
>> automatically? If not, I would be strongly against it, since:
>>
>> - Intellij’s automatic refactor actions will not work properly.
>> - I don’t think it’s a big deal. I don’t remember having any issues with
>> the lack or presence of the `final` keyword.
>> - `final` is pretty much useless in most of the cases (it’s not `const`
>> and it works only for the primitive types).
>> - I don’t like the extra overhead of doing something of very little extra
>> value. Especially the extra hassle of going back & forth during the reviews
>> (both as a contributor & as a reviewer).
>> - If missing `final` keyword caused some confusion, because surprisingly a
>> parameter was modified somewhere in the function and it wasn’t obviously
>> visible, the function is doing probably too many things and it’s too
>> long/too complicated…
>>
>> Generally speaking, I’m against adding minor things to our codestyle that
>> can not be enforced and added automatically.
>>
>> Piotrek
>>
>>> On 7 Oct 2019, at 09:13, Arvid Heise <ar...@ververica.com> wrote:
>>>
>>> Hi guys,
>>>
>>> I'm a bit torn. In general, +1 for making parameters effectively final.
>>>
>>> The question is how to enforce it. We can make it explicit (like Aljoscha
>>> said). All IDEs will show immediately warnings/errors for violations. It
>>> would allow to softly migrate code.
>>>
>>> Another option is to use a checkstyle rule [1]. Then, we could omit the
>>> final keyword and rely on checkstyle checks as we do for quite a few
>> other
>>> things. A hard checkstyle rule would probably fail on a good portion of
>> the
>>> current code base. But we would also remove reassignment to parameters
>>> (which I consider an anti-pattern).
>>>
>>> If we opt not to enforce it, then -1 for removing final keywords from my
>>> side.
>>>
>>> Best,
>>>
>>> Arvid
>>>
>>> [1]
>>>
>> https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/checks/coding/ParameterAssignmentCheck.html
>>> On Fri, Oct 4, 2019 at 1:22 PM Zili Chen <wa...@gmail.com> wrote:
>>>
>>>> Hi Aljoscha,
>>>>
>>>> I totally agree with you on field topic. Of course it makes significant
>>>> difference whether
>>>> or not a field is final and IDE/compiler can help on checking.
>>>>
>>>> Here are several thoughts about final modifier on parameters and why I
>>>> propose this one:
>>>>
>>>> 1. parameters should be always final
>>>>
>>>> As described above, there is no reason a parameter to be non-final. So
>>>> different with field,
>>>> a field can be final or non-final based on whether or not it is
>> immutable.
>>>> Thus with such a
>>>> code style guide in our community, we can work towards a codebase every
>>>> parameter is
>>>> effectively final.
>>>>
>>>> 2. parameter final cannot be inherited
>>>>
>>>> Unfortunately Java doesn't keep final modifier of method parameter when
>>>> inheritance. So
>>>> even you mark a parameter as final in an interface or super class, you
>> have
>>>> to re-mark it
>>>> as final in its implementation or subclass. From another perspective,
>> final
>>>> modifier of
>>>> parameter is a local attribute of parameter so that we can narrow
>> possible
>>>> effect during
>>>> review.
>>>>
>>>> 3. IDE can lint difference between effective final and mutable parameter
>>>>
>>>> It is true that IDE such as Intellij IDEA can lint difference between
>>>> effective final and
>>>> mutable parameter(with underline). So that with this code style what we
>>>> lose is that
>>>> we cannot get a compile time error if someone modifies parameter in the
>>>> method body.
>>>> But as mentioned in 1, by no means we allow a parameter to be modified.
>> If
>>>> we agree
>>>> on this statement, then we hopefully converge in a codebase that no
>>>> parameter is
>>>> modified.
>>>>
>>>> For what we gain, I'd like to recur our existing code style of @Nonnull
>> to
>>>> be default.
>>>> Actually it does help for compiler to report compile time warning if we
>>>> possibly pass a
>>>> nullable value to an non-null field. We make @Nonnull as default to
>> "reduce
>>>> code noise"
>>>> so I think we can reuse the statement here also.
>>>>
>>>> Best,
>>>> tison.
>>>>
>>>>
>>>> Aljoscha Krettek <al...@apache.org> 于2019年10月4日周五 下午5:58写道：
>>>>
>>>>> I actually think that doing this the other way round would be correct.
>>>>> Removing final everywhere and relying on humans to assume that
>> everything
>>>>> is final doesn’t seem maintainable to me. The benefit of having final
>> on
>>>>> parameters/fields is that the compiler/IDE actually checks that you
>> don’t
>>>>> modify it.
>>>>>
>>>>> In general, I think that as much as possible should be declared final,
>>>>> including fields and parameters.
>>>>>
>>>>> Best,
>>>>> Aljoscha
>>>>>
>>>>>> On 2. Oct 2019, at 13:31, Piotr Nowojski <pi...@ververica.com> wrote:
>>>>>>
>>>>>> +1 from my side.
>>>>>>
>>>>>>> On 2 Oct 2019, at 13:07, Zili Chen <wa...@gmail.com> wrote:
>>>>>>>
>>>>>>> Yes exactly.
>>>>>>>
>>>>>>>
>>>>>>> Piotr Nowojski <pi...@ververica.com> 于2019年10月2日周三 下午7:03写道：
>>>>>>>
>>>>>>>> Hi Tison,
>>>>>>>>
>>>>>>>> To clarify      your proposal. You are proposing to actually drop
>> the
>>>>>>>> `final` keyword from the parameters and we should implicilty assume
>>>>> that
>>>>>>>> it’s always there (in other words, we shouldn’t be modifying the
>>>>>>>> parameters). Did I understand this correctly?
>>>>>>>>
>>>>>>>> Piotrek
>>>>>>>>
>>>>>>>>> On 1 Oct 2019, at 21:44, Zili Chen <wa...@gmail.com> wrote:
>>>>>>>>>
>>>>>>>>> Hi devs,
>>>>>>>>>
>>>>>>>>> Coming from this discussion[1] I'd like to propose that in Flink
>>>>> codebase
>>>>>>>>> we suggest a code style
>>>>>>>>> that parameters of method are always final. Almost everywhere
>>>>> parameters
>>>>>>>> of
>>>>>>>>> method are final
>>>>>>>>> already and if we have such consensus we can prevent redundant
>> final
>>>>>>>>> modifier in method
>>>>>>>>> declaration so that we survive from those noise.
>>>>>>>>>
>>>>>>>>> Here are some cases that might require to modify a parameter.
>>>>>>>>>
>>>>>>>>> 1. to set default; especially if (param == null) { param = ... }
>>>>>>>>> 2. to refine parameter; it is in pattern if ( ... ) { param =
>>>>>>>>> refine(param); }
>>>>>>>>>
>>>>>>>>> Either of the cases we can move the refine/set default logics to
>> the
>>>>>>>> caller
>>>>>>>>> or select another
>>>>>>>>> name for the refined value case by case.
>>>>>>>>>
>>>>>>>>> Looking forward to your feedbacks :-)
>>>>>>>>>
>>>>>>>>> Best,
>>>>>>>>> tison.
>>>>>>>>>
>>>>>>>>> [1]
>> https://github.com/apache/flink/pull/9788#discussion_r329314681
>>>>>>>>
>>>>>
>>

Re: [CODE STYLE] Parameters of method are always final

Posted by Zili Chen <wa...@gmail.com>.

Thanks for your thoughts Arvid & Piotr,

I check out the effect of ParameterAssignment[1] and it does
prevent codes from modifying parameter which I argued above
the most value introduced by `final` modifier of parameter.

So firstly, I think it's good to enable ParameterAssignment in our
codebase.

Besides, there is no rule to forbid `final` modifier of parameter.
Instead, there is a rule to enforce `final` modifier[2] but given [1]
enabled it is actually redundant.

The main purpose for, given enforced not modify parameters, reducing
`final` modifiers of parameter is to remove redundant modifier so that we
don't see have declaration like

T fn(
  final ArgumentType1 argument1,
  final ArgumentType2 argument2,
  ...)

because we actually don't mark final everywhere so it might make some
confusions.

Given [1] is enforced these `final` are indeed redundant so that we can
add a convention to reduce `final` modifiers of parameters, which is a net
win.

Best,
tison.

[1] https://checkstyle.sourceforge.io/config_coding.html#ParameterAssignment
[2] https://checkstyle.sourceforge.io/config_misc.html#FinalParameters



Piotr Nowojski <pi...@ververica.com> 于2019年10月7日周一 下午3:49写道：

> Hi,
>
> Couple of arguments to counter the proposal of making the `final` keyword
> obligatory. Can we prepare a code style/IDE settings to add it
> automatically? If not, I would be strongly against it, since:
>
> - Intellij’s automatic refactor actions will not work properly.
> - I don’t think it’s a big deal. I don’t remember having any issues with
> the lack or presence of the `final` keyword.
> - `final` is pretty much useless in most of the cases (it’s not `const`
> and it works only for the primitive types).
> - I don’t like the extra overhead of doing something of very little extra
> value. Especially the extra hassle of going back & forth during the reviews
> (both as a contributor & as a reviewer).
> - If missing `final` keyword caused some confusion, because surprisingly a
> parameter was modified somewhere in the function and it wasn’t obviously
> visible, the function is doing probably too many things and it’s too
> long/too complicated…
>
> Generally speaking, I’m against adding minor things to our codestyle that
> can not be enforced and added automatically.
>
> Piotrek
>
> > On 7 Oct 2019, at 09:13, Arvid Heise <ar...@ververica.com> wrote:
> >
> > Hi guys,
> >
> > I'm a bit torn. In general, +1 for making parameters effectively final.
> >
> > The question is how to enforce it. We can make it explicit (like Aljoscha
> > said). All IDEs will show immediately warnings/errors for violations. It
> > would allow to softly migrate code.
> >
> > Another option is to use a checkstyle rule [1]. Then, we could omit the
> > final keyword and rely on checkstyle checks as we do for quite a few
> other
> > things. A hard checkstyle rule would probably fail on a good portion of
> the
> > current code base. But we would also remove reassignment to parameters
> > (which I consider an anti-pattern).
> >
> > If we opt not to enforce it, then -1 for removing final keywords from my
> > side.
> >
> > Best,
> >
> > Arvid
> >
> > [1]
> >
> https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/checks/coding/ParameterAssignmentCheck.html
> >
> > On Fri, Oct 4, 2019 at 1:22 PM Zili Chen <wa...@gmail.com> wrote:
> >
> >> Hi Aljoscha,
> >>
> >> I totally agree with you on field topic. Of course it makes significant
> >> difference whether
> >> or not a field is final and IDE/compiler can help on checking.
> >>
> >> Here are several thoughts about final modifier on parameters and why I
> >> propose this one:
> >>
> >> 1. parameters should be always final
> >>
> >> As described above, there is no reason a parameter to be non-final. So
> >> different with field,
> >> a field can be final or non-final based on whether or not it is
> immutable.
> >> Thus with such a
> >> code style guide in our community, we can work towards a codebase every
> >> parameter is
> >> effectively final.
> >>
> >> 2. parameter final cannot be inherited
> >>
> >> Unfortunately Java doesn't keep final modifier of method parameter when
> >> inheritance. So
> >> even you mark a parameter as final in an interface or super class, you
> have
> >> to re-mark it
> >> as final in its implementation or subclass. From another perspective,
> final
> >> modifier of
> >> parameter is a local attribute of parameter so that we can narrow
> possible
> >> effect during
> >> review.
> >>
> >> 3. IDE can lint difference between effective final and mutable parameter
> >>
> >> It is true that IDE such as Intellij IDEA can lint difference between
> >> effective final and
> >> mutable parameter(with underline). So that with this code style what we
> >> lose is that
> >> we cannot get a compile time error if someone modifies parameter in the
> >> method body.
> >> But as mentioned in 1, by no means we allow a parameter to be modified.
> If
> >> we agree
> >> on this statement, then we hopefully converge in a codebase that no
> >> parameter is
> >> modified.
> >>
> >> For what we gain, I'd like to recur our existing code style of @Nonnull
> to
> >> be default.
> >> Actually it does help for compiler to report compile time warning if we
> >> possibly pass a
> >> nullable value to an non-null field. We make @Nonnull as default to
> "reduce
> >> code noise"
> >> so I think we can reuse the statement here also.
> >>
> >> Best,
> >> tison.
> >>
> >>
> >> Aljoscha Krettek <al...@apache.org> 于2019年10月4日周五 下午5:58写道：
> >>
> >>> I actually think that doing this the other way round would be correct.
> >>> Removing final everywhere and relying on humans to assume that
> everything
> >>> is final doesn’t seem maintainable to me. The benefit of having final
> on
> >>> parameters/fields is that the compiler/IDE actually checks that you
> don’t
> >>> modify it.
> >>>
> >>> In general, I think that as much as possible should be declared final,
> >>> including fields and parameters.
> >>>
> >>> Best,
> >>> Aljoscha
> >>>
> >>>> On 2. Oct 2019, at 13:31, Piotr Nowojski <pi...@ververica.com> wrote:
> >>>>
> >>>> +1 from my side.
> >>>>
> >>>>> On 2 Oct 2019, at 13:07, Zili Chen <wa...@gmail.com> wrote:
> >>>>>
> >>>>> Yes exactly.
> >>>>>
> >>>>>
> >>>>> Piotr Nowojski <pi...@ververica.com> 于2019年10月2日周三 下午7:03写道：
> >>>>>
> >>>>>> Hi Tison,
> >>>>>>
> >>>>>> To clarify      your proposal. You are proposing to actually drop
> the
> >>>>>> `final` keyword from the parameters and we should implicilty assume
> >>> that
> >>>>>> it’s always there (in other words, we shouldn’t be modifying the
> >>>>>> parameters). Did I understand this correctly?
> >>>>>>
> >>>>>> Piotrek
> >>>>>>
> >>>>>>> On 1 Oct 2019, at 21:44, Zili Chen <wa...@gmail.com> wrote:
> >>>>>>>
> >>>>>>> Hi devs,
> >>>>>>>
> >>>>>>> Coming from this discussion[1] I'd like to propose that in Flink
> >>> codebase
> >>>>>>> we suggest a code style
> >>>>>>> that parameters of method are always final. Almost everywhere
> >>> parameters
> >>>>>> of
> >>>>>>> method are final
> >>>>>>> already and if we have such consensus we can prevent redundant
> final
> >>>>>>> modifier in method
> >>>>>>> declaration so that we survive from those noise.
> >>>>>>>
> >>>>>>> Here are some cases that might require to modify a parameter.
> >>>>>>>
> >>>>>>> 1. to set default; especially if (param == null) { param = ... }
> >>>>>>> 2. to refine parameter; it is in pattern if ( ... ) { param =
> >>>>>>> refine(param); }
> >>>>>>>
> >>>>>>> Either of the cases we can move the refine/set default logics to
> the
> >>>>>> caller
> >>>>>>> or select another
> >>>>>>> name for the refined value case by case.
> >>>>>>>
> >>>>>>> Looking forward to your feedbacks :-)
> >>>>>>>
> >>>>>>> Best,
> >>>>>>> tison.
> >>>>>>>
> >>>>>>> [1]
> https://github.com/apache/flink/pull/9788#discussion_r329314681
> >>>>>>
> >>>>>>
> >>>>
> >>>
> >>>
> >>
>
>

Re: [CODE STYLE] Parameters of method are always final

Posted by Piotr Nowojski <pi...@ververica.com>.

Hi,

Couple of arguments to counter the proposal of making the `final` keyword obligatory. Can we prepare a code style/IDE settings to add it automatically? If not, I would be strongly against it, since:

- Intellij’s automatic refactor actions will not work properly.
- I don’t think it’s a big deal. I don’t remember having any issues with the lack or presence of the `final` keyword.
- `final` is pretty much useless in most of the cases (it’s not `const` and it works only for the primitive types).
- I don’t like the extra overhead of doing something of very little extra value. Especially the extra hassle of going back & forth during the reviews (both as a contributor & as a reviewer).
- If missing `final` keyword caused some confusion, because surprisingly a parameter was modified somewhere in the function and it wasn’t obviously visible, the function is doing probably too many things and it’s too long/too complicated…

Generally speaking, I’m against adding minor things to our codestyle that can not be enforced and added automatically.

Piotrek

> On 7 Oct 2019, at 09:13, Arvid Heise <ar...@ververica.com> wrote:
> 
> Hi guys,
> 
> I'm a bit torn. In general, +1 for making parameters effectively final.
> 
> The question is how to enforce it. We can make it explicit (like Aljoscha
> said). All IDEs will show immediately warnings/errors for violations. It
> would allow to softly migrate code.
> 
> Another option is to use a checkstyle rule [1]. Then, we could omit the
> final keyword and rely on checkstyle checks as we do for quite a few other
> things. A hard checkstyle rule would probably fail on a good portion of the
> current code base. But we would also remove reassignment to parameters
> (which I consider an anti-pattern).
> 
> If we opt not to enforce it, then -1 for removing final keywords from my
> side.
> 
> Best,
> 
> Arvid
> 
> [1]
> https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/checks/coding/ParameterAssignmentCheck.html
> 
> On Fri, Oct 4, 2019 at 1:22 PM Zili Chen <wa...@gmail.com> wrote:
> 
>> Hi Aljoscha,
>> 
>> I totally agree with you on field topic. Of course it makes significant
>> difference whether
>> or not a field is final and IDE/compiler can help on checking.
>> 
>> Here are several thoughts about final modifier on parameters and why I
>> propose this one:
>> 
>> 1. parameters should be always final
>> 
>> As described above, there is no reason a parameter to be non-final. So
>> different with field,
>> a field can be final or non-final based on whether or not it is immutable.
>> Thus with such a
>> code style guide in our community, we can work towards a codebase every
>> parameter is
>> effectively final.
>> 
>> 2. parameter final cannot be inherited
>> 
>> Unfortunately Java doesn't keep final modifier of method parameter when
>> inheritance. So
>> even you mark a parameter as final in an interface or super class, you have
>> to re-mark it
>> as final in its implementation or subclass. From another perspective, final
>> modifier of
>> parameter is a local attribute of parameter so that we can narrow possible
>> effect during
>> review.
>> 
>> 3. IDE can lint difference between effective final and mutable parameter
>> 
>> It is true that IDE such as Intellij IDEA can lint difference between
>> effective final and
>> mutable parameter(with underline). So that with this code style what we
>> lose is that
>> we cannot get a compile time error if someone modifies parameter in the
>> method body.
>> But as mentioned in 1, by no means we allow a parameter to be modified. If
>> we agree
>> on this statement, then we hopefully converge in a codebase that no
>> parameter is
>> modified.
>> 
>> For what we gain, I'd like to recur our existing code style of @Nonnull to
>> be default.
>> Actually it does help for compiler to report compile time warning if we
>> possibly pass a
>> nullable value to an non-null field. We make @Nonnull as default to "reduce
>> code noise"
>> so I think we can reuse the statement here also.
>> 
>> Best,
>> tison.
>> 
>> 
>> Aljoscha Krettek <al...@apache.org> 于2019年10月4日周五 下午5:58写道：
>> 
>>> I actually think that doing this the other way round would be correct.
>>> Removing final everywhere and relying on humans to assume that everything
>>> is final doesn’t seem maintainable to me. The benefit of having final on
>>> parameters/fields is that the compiler/IDE actually checks that you don’t
>>> modify it.
>>> 
>>> In general, I think that as much as possible should be declared final,
>>> including fields and parameters.
>>> 
>>> Best,
>>> Aljoscha
>>> 
>>>> On 2. Oct 2019, at 13:31, Piotr Nowojski <pi...@ververica.com> wrote:
>>>> 
>>>> +1 from my side.
>>>> 
>>>>> On 2 Oct 2019, at 13:07, Zili Chen <wa...@gmail.com> wrote:
>>>>> 
>>>>> Yes exactly.
>>>>> 
>>>>> 
>>>>> Piotr Nowojski <pi...@ververica.com> 于2019年10月2日周三 下午7:03写道：
>>>>> 
>>>>>> Hi Tison,
>>>>>> 
>>>>>> To clarify      your proposal. You are proposing to actually drop the
>>>>>> `final` keyword from the parameters and we should implicilty assume
>>> that
>>>>>> it’s always there (in other words, we shouldn’t be modifying the
>>>>>> parameters). Did I understand this correctly?
>>>>>> 
>>>>>> Piotrek
>>>>>> 
>>>>>>> On 1 Oct 2019, at 21:44, Zili Chen <wa...@gmail.com> wrote:
>>>>>>> 
>>>>>>> Hi devs,
>>>>>>> 
>>>>>>> Coming from this discussion[1] I'd like to propose that in Flink
>>> codebase
>>>>>>> we suggest a code style
>>>>>>> that parameters of method are always final. Almost everywhere
>>> parameters
>>>>>> of
>>>>>>> method are final
>>>>>>> already and if we have such consensus we can prevent redundant final
>>>>>>> modifier in method
>>>>>>> declaration so that we survive from those noise.
>>>>>>> 
>>>>>>> Here are some cases that might require to modify a parameter.
>>>>>>> 
>>>>>>> 1. to set default; especially if (param == null) { param = ... }
>>>>>>> 2. to refine parameter; it is in pattern if ( ... ) { param =
>>>>>>> refine(param); }
>>>>>>> 
>>>>>>> Either of the cases we can move the refine/set default logics to the
>>>>>> caller
>>>>>>> or select another
>>>>>>> name for the refined value case by case.
>>>>>>> 
>>>>>>> Looking forward to your feedbacks :-)
>>>>>>> 
>>>>>>> Best,
>>>>>>> tison.
>>>>>>> 
>>>>>>> [1] https://github.com/apache/flink/pull/9788#discussion_r329314681
>>>>>> 
>>>>>> 
>>>> 
>>> 
>>> 
>>

Re: [CODE STYLE] Parameters of method are always final

Posted by Arvid Heise <ar...@ververica.com>.

Hi guys,

I'm a bit torn. In general, +1 for making parameters effectively final.

The question is how to enforce it. We can make it explicit (like Aljoscha
said). All IDEs will show immediately warnings/errors for violations. It
would allow to softly migrate code.

Another option is to use a checkstyle rule [1]. Then, we could omit the
final keyword and rely on checkstyle checks as we do for quite a few other
things. A hard checkstyle rule would probably fail on a good portion of the
current code base. But we would also remove reassignment to parameters
(which I consider an anti-pattern).

If we opt not to enforce it, then -1 for removing final keywords from my
side.

Best,

Arvid

[1]
https://checkstyle.sourceforge.io/apidocs/com/puppycrawl/tools/checkstyle/checks/coding/ParameterAssignmentCheck.html

On Fri, Oct 4, 2019 at 1:22 PM Zili Chen <wa...@gmail.com> wrote:

> Hi Aljoscha,
>
> I totally agree with you on field topic. Of course it makes significant
> difference whether
> or not a field is final and IDE/compiler can help on checking.
>
> Here are several thoughts about final modifier on parameters and why I
> propose this one:
>
> 1. parameters should be always final
>
> As described above, there is no reason a parameter to be non-final. So
> different with field,
> a field can be final or non-final based on whether or not it is immutable.
> Thus with such a
> code style guide in our community, we can work towards a codebase every
> parameter is
> effectively final.
>
> 2. parameter final cannot be inherited
>
> Unfortunately Java doesn't keep final modifier of method parameter when
> inheritance. So
> even you mark a parameter as final in an interface or super class, you have
> to re-mark it
> as final in its implementation or subclass. From another perspective, final
> modifier of
> parameter is a local attribute of parameter so that we can narrow possible
> effect during
> review.
>
> 3. IDE can lint difference between effective final and mutable parameter
>
> It is true that IDE such as Intellij IDEA can lint difference between
> effective final and
> mutable parameter(with underline). So that with this code style what we
> lose is that
> we cannot get a compile time error if someone modifies parameter in the
> method body.
> But as mentioned in 1, by no means we allow a parameter to be modified. If
> we agree
> on this statement, then we hopefully converge in a codebase that no
> parameter is
> modified.
>
> For what we gain, I'd like to recur our existing code style of @Nonnull to
> be default.
> Actually it does help for compiler to report compile time warning if we
> possibly pass a
> nullable value to an non-null field. We make @Nonnull as default to "reduce
> code noise"
> so I think we can reuse the statement here also.
>
> Best,
> tison.
>
>
> Aljoscha Krettek <al...@apache.org> 于2019年10月4日周五 下午5:58写道：
>
> > I actually think that doing this the other way round would be correct.
> > Removing final everywhere and relying on humans to assume that everything
> > is final doesn’t seem maintainable to me. The benefit of having final on
> > parameters/fields is that the compiler/IDE actually checks that you don’t
> > modify it.
> >
> > In general, I think that as much as possible should be declared final,
> > including fields and parameters.
> >
> > Best,
> > Aljoscha
> >
> > > On 2. Oct 2019, at 13:31, Piotr Nowojski <pi...@ververica.com> wrote:
> > >
> > > +1 from my side.
> > >
> > >> On 2 Oct 2019, at 13:07, Zili Chen <wa...@gmail.com> wrote:
> > >>
> > >> Yes exactly.
> > >>
> > >>
> > >> Piotr Nowojski <pi...@ververica.com> 于2019年10月2日周三 下午7:03写道：
> > >>
> > >>> Hi Tison,
> > >>>
> > >>> To clarify      your proposal. You are proposing to actually drop the
> > >>> `final` keyword from the parameters and we should implicilty assume
> > that
> > >>> it’s always there (in other words, we shouldn’t be modifying the
> > >>> parameters). Did I understand this correctly?
> > >>>
> > >>> Piotrek
> > >>>
> > >>>> On 1 Oct 2019, at 21:44, Zili Chen <wa...@gmail.com> wrote:
> > >>>>
> > >>>> Hi devs,
> > >>>>
> > >>>> Coming from this discussion[1] I'd like to propose that in Flink
> > codebase
> > >>>> we suggest a code style
> > >>>> that parameters of method are always final. Almost everywhere
> > parameters
> > >>> of
> > >>>> method are final
> > >>>> already and if we have such consensus we can prevent redundant final
> > >>>> modifier in method
> > >>>> declaration so that we survive from those noise.
> > >>>>
> > >>>> Here are some cases that might require to modify a parameter.
> > >>>>
> > >>>> 1. to set default; especially if (param == null) { param = ... }
> > >>>> 2. to refine parameter; it is in pattern if ( ... ) { param =
> > >>>> refine(param); }
> > >>>>
> > >>>> Either of the cases we can move the refine/set default logics to the
> > >>> caller
> > >>>> or select another
> > >>>> name for the refined value case by case.
> > >>>>
> > >>>> Looking forward to your feedbacks :-)
> > >>>>
> > >>>> Best,
> > >>>> tison.
> > >>>>
> > >>>> [1] https://github.com/apache/flink/pull/9788#discussion_r329314681
> > >>>
> > >>>
> > >
> >
> >
>

Re: [CODE STYLE] Parameters of method are always final

Posted by Zili Chen <wa...@gmail.com>.

Hi Aljoscha,

I totally agree with you on field topic. Of course it makes significant
difference whether
or not a field is final and IDE/compiler can help on checking.

Here are several thoughts about final modifier on parameters and why I
propose this one:

1. parameters should be always final

As described above, there is no reason a parameter to be non-final. So
different with field,
a field can be final or non-final based on whether or not it is immutable.
Thus with such a
code style guide in our community, we can work towards a codebase every
parameter is
effectively final.

2. parameter final cannot be inherited

Unfortunately Java doesn't keep final modifier of method parameter when
inheritance. So
even you mark a parameter as final in an interface or super class, you have
to re-mark it
as final in its implementation or subclass. From another perspective, final
modifier of
parameter is a local attribute of parameter so that we can narrow possible
effect during
review.

3. IDE can lint difference between effective final and mutable parameter

It is true that IDE such as Intellij IDEA can lint difference between
effective final and
mutable parameter(with underline). So that with this code style what we
lose is that
we cannot get a compile time error if someone modifies parameter in the
method body.
But as mentioned in 1, by no means we allow a parameter to be modified. If
we agree
on this statement, then we hopefully converge in a codebase that no
parameter is
modified.

For what we gain, I'd like to recur our existing code style of @Nonnull to
be default.
Actually it does help for compiler to report compile time warning if we
possibly pass a
nullable value to an non-null field. We make @Nonnull as default to "reduce
code noise"
so I think we can reuse the statement here also.

Best,
tison.

Aljoscha Krettek <al...@apache.org> 于2019年10月4日周五 下午5:58写道：

> I actually think that doing this the other way round would be correct.
> Removing final everywhere and relying on humans to assume that everything
> is final doesn’t seem maintainable to me. The benefit of having final on
> parameters/fields is that the compiler/IDE actually checks that you don’t
> modify it.
>
> In general, I think that as much as possible should be declared final,
> including fields and parameters.
>
> Best,
> Aljoscha
>
> > On 2. Oct 2019, at 13:31, Piotr Nowojski <pi...@ververica.com> wrote:
> >
> > +1 from my side.
> >
> >> On 2 Oct 2019, at 13:07, Zili Chen <wa...@gmail.com> wrote:
> >>
> >> Yes exactly.
> >>
> >>
> >> Piotr Nowojski <pi...@ververica.com> 于2019年10月2日周三 下午7:03写道：
> >>
> >>> Hi Tison,
> >>>
> >>> To clarify      your proposal. You are proposing to actually drop the
> >>> `final` keyword from the parameters and we should implicilty assume
> that
> >>> it’s always there (in other words, we shouldn’t be modifying the
> >>> parameters). Did I understand this correctly?
> >>>
> >>> Piotrek
> >>>
> >>>> On 1 Oct 2019, at 21:44, Zili Chen <wa...@gmail.com> wrote:
> >>>>
> >>>> Hi devs,
> >>>>
> >>>> Coming from this discussion[1] I'd like to propose that in Flink
> codebase
> >>>> we suggest a code style
> >>>> that parameters of method are always final. Almost everywhere
> parameters
> >>> of
> >>>> method are final
> >>>> already and if we have such consensus we can prevent redundant final
> >>>> modifier in method
> >>>> declaration so that we survive from those noise.
> >>>>
> >>>> Here are some cases that might require to modify a parameter.
> >>>>
> >>>> 1. to set default; especially if (param == null) { param = ... }
> >>>> 2. to refine parameter; it is in pattern if ( ... ) { param =
> >>>> refine(param); }
> >>>>
> >>>> Either of the cases we can move the refine/set default logics to the
> >>> caller
> >>>> or select another
> >>>> name for the refined value case by case.
> >>>>
> >>>> Looking forward to your feedbacks :-)
> >>>>
> >>>> Best,
> >>>> tison.
> >>>>
> >>>> [1] https://github.com/apache/flink/pull/9788#discussion_r329314681
> >>>
> >>>
> >
>
>

Re: [CODE STYLE] Parameters of method are always final

Posted by Aljoscha Krettek <al...@apache.org>.

I actually think that doing this the other way round would be correct. Removing final everywhere and relying on humans to assume that everything is final doesn’t seem maintainable to me. The benefit of having final on parameters/fields is that the compiler/IDE actually checks that you don’t modify it.

In general, I think that as much as possible should be declared final, including fields and parameters.

Best,
Aljoscha

> On 2. Oct 2019, at 13:31, Piotr Nowojski <pi...@ververica.com> wrote:
> 
> +1 from my side.
> 
>> On 2 Oct 2019, at 13:07, Zili Chen <wa...@gmail.com> wrote:
>> 
>> Yes exactly.
>> 
>> 
>> Piotr Nowojski <pi...@ververica.com> 于2019年10月2日周三 下午7:03写道：
>> 
>>> Hi Tison,
>>> 
>>> To clarify      your proposal. You are proposing to actually drop the
>>> `final` keyword from the parameters and we should implicilty assume that
>>> it’s always there (in other words, we shouldn’t be modifying the
>>> parameters). Did I understand this correctly?
>>> 
>>> Piotrek
>>> 
>>>> On 1 Oct 2019, at 21:44, Zili Chen <wa...@gmail.com> wrote:
>>>> 
>>>> Hi devs,
>>>> 
>>>> Coming from this discussion[1] I'd like to propose that in Flink codebase
>>>> we suggest a code style
>>>> that parameters of method are always final. Almost everywhere parameters
>>> of
>>>> method are final
>>>> already and if we have such consensus we can prevent redundant final
>>>> modifier in method
>>>> declaration so that we survive from those noise.
>>>> 
>>>> Here are some cases that might require to modify a parameter.
>>>> 
>>>> 1. to set default; especially if (param == null) { param = ... }
>>>> 2. to refine parameter; it is in pattern if ( ... ) { param =
>>>> refine(param); }
>>>> 
>>>> Either of the cases we can move the refine/set default logics to the
>>> caller
>>>> or select another
>>>> name for the refined value case by case.
>>>> 
>>>> Looking forward to your feedbacks :-)
>>>> 
>>>> Best,
>>>> tison.
>>>> 
>>>> [1] https://github.com/apache/flink/pull/9788#discussion_r329314681
>>> 
>>> 
>

Re: [CODE STYLE] Parameters of method are always final

Posted by Piotr Nowojski <pi...@ververica.com>.

+1 from my side.

> On 2 Oct 2019, at 13:07, Zili Chen <wa...@gmail.com> wrote:
> 
> Yes exactly.
> 
> 
> Piotr Nowojski <pi...@ververica.com> 于2019年10月2日周三 下午7:03写道：
> 
>> Hi Tison,
>> 
>> To clarify      your proposal. You are proposing to actually drop the
>> `final` keyword from the parameters and we should implicilty assume that
>> it’s always there (in other words, we shouldn’t be modifying the
>> parameters). Did I understand this correctly?
>> 
>> Piotrek
>> 
>>> On 1 Oct 2019, at 21:44, Zili Chen <wa...@gmail.com> wrote:
>>> 
>>> Hi devs,
>>> 
>>> Coming from this discussion[1] I'd like to propose that in Flink codebase
>>> we suggest a code style
>>> that parameters of method are always final. Almost everywhere parameters
>> of
>>> method are final
>>> already and if we have such consensus we can prevent redundant final
>>> modifier in method
>>> declaration so that we survive from those noise.
>>> 
>>> Here are some cases that might require to modify a parameter.
>>> 
>>> 1. to set default; especially if (param == null) { param = ... }
>>> 2. to refine parameter; it is in pattern if ( ... ) { param =
>>> refine(param); }
>>> 
>>> Either of the cases we can move the refine/set default logics to the
>> caller
>>> or select another
>>> name for the refined value case by case.
>>> 
>>> Looking forward to your feedbacks :-)
>>> 
>>> Best,
>>> tison.
>>> 
>>> [1] https://github.com/apache/flink/pull/9788#discussion_r329314681
>> 
>>

Re: [CODE STYLE] Parameters of method are always final

Posted by Zili Chen <wa...@gmail.com>.

Yes exactly.


Piotr Nowojski <pi...@ververica.com> 于2019年10月2日周三 下午7:03写道：

> Hi Tison,
>
> To clarify      your proposal. You are proposing to actually drop the
> `final` keyword from the parameters and we should implicilty assume that
> it’s always there (in other words, we shouldn’t be modifying the
> parameters). Did I understand this correctly?
>
> Piotrek
>
> > On 1 Oct 2019, at 21:44, Zili Chen <wa...@gmail.com> wrote:
> >
> > Hi devs,
> >
> > Coming from this discussion[1] I'd like to propose that in Flink codebase
> > we suggest a code style
> > that parameters of method are always final. Almost everywhere parameters
> of
> > method are final
> > already and if we have such consensus we can prevent redundant final
> > modifier in method
> > declaration so that we survive from those noise.
> >
> > Here are some cases that might require to modify a parameter.
> >
> > 1. to set default; especially if (param == null) { param = ... }
> > 2. to refine parameter; it is in pattern if ( ... ) { param =
> > refine(param); }
> >
> > Either of the cases we can move the refine/set default logics to the
> caller
> > or select another
> > name for the refined value case by case.
> >
> > Looking forward to your feedbacks :-)
> >
> > Best,
> > tison.
> >
> > [1] https://github.com/apache/flink/pull/9788#discussion_r329314681
>
>

Re: [CODE STYLE] Parameters of method are always final

Posted by Piotr Nowojski <pi...@ververica.com>.

Hi Tison,

To clarify	your proposal. You are proposing to actually drop the `final` keyword from the parameters and we should implicilty assume that it’s always there (in other words, we shouldn’t be modifying the parameters). Did I understand this correctly?

Piotrek 

> On 1 Oct 2019, at 21:44, Zili Chen <wa...@gmail.com> wrote:
> 
> Hi devs,
> 
> Coming from this discussion[1] I'd like to propose that in Flink codebase
> we suggest a code style
> that parameters of method are always final. Almost everywhere parameters of
> method are final
> already and if we have such consensus we can prevent redundant final
> modifier in method
> declaration so that we survive from those noise.
> 
> Here are some cases that might require to modify a parameter.
> 
> 1. to set default; especially if (param == null) { param = ... }
> 2. to refine parameter; it is in pattern if ( ... ) { param =
> refine(param); }
> 
> Either of the cases we can move the refine/set default logics to the caller
> or select another
> name for the refined value case by case.
> 
> Looking forward to your feedbacks :-)
> 
> Best,
> tison.
> 
> [1] https://github.com/apache/flink/pull/9788#discussion_r329314681