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 > > >>> > > >>> > > > > > > > >
