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 <wander4...@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 <aljos...@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 <wander4...@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 <wander4...@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 >>>>>> >>>>>> >>>> >>> >>> >>
