Bugzilla – Full Text Bug Listing
| Summary: | Coding conventions: Remove final keywords for parameters | ||
|---|---|---|---|
| Product: | [RT] RAP | Reporter: | Ralf Sternberg <rsternberg> |
| Component: | RWT | Assignee: | Project Inbox <rap-inbox> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P2 | CC: | austin.riddle, christian.campo, fr.appel, gunnar, holger.staudacher, ruediger.herrmann |
| Version: | 1.4 | ||
| Target Milestone: | 1.5 | ||
| Hardware: | PC | ||
| OS: | Linux | ||
| Whiteboard: | |||
|
Description
Ralf Sternberg
+1 for option b) +1 for option b +1 for option b) seems to be the best compromise in this case. Just for curiosity. When you started the description you obviously already decided that you want to remove final. I wondering why ? We in Riena just a couple of month ago decided the opposite and converted all our code. So now all variables (instant vars, local vars, parameters) are always final unless they are modified in the code. I believe there is a setting for that in the code and it is applied every time you save. And you cant assign something to a parameter. It seems (from the outside) that you actually want this behaviour but dont want the "final" word in your code and now you are working around this problem with tooling. So I am curious to learn why you choose otherwise. Why would we make the tooling flag a parameter assignment as a warning and the build server as an error? It seems like it would be better to make the tooling flag an error instead. (In reply to comment #4) The coding conventions of the RAP team are based on the original coding conventions we used for the RAP ancestor W4Toolkit (for me it feels like this was somewhere shortly after the french revolution..). The initial thought on having the final keyword on every parameter was to avoid the misuse of parameters as local variables. So once you have those finals in place one may argue (at least I did) that the content of the method block will be clean of such misuse. But as the years went by I changed my view point a little bit: The final keyword in parameters leads to less readable method headers - at least if you have method names that are a little bit longer and the parameter list exceeds your line limit. So you'll have to wrap the parameter list often even if the given method only uses two parameters. Given that nowadays IDEs are able to warn you about such parameter misuse I favor the more readable method header without final keywords. By the way: I guess if one have a team of programmers that are prepared to misuse parameters as local variables without getting the bad smell of it, he will be in much more trouble than he can fix with coding conventions - IDE enforced or not ;-) (In reply to comment #5) This was my initial thought too, but it would increase the effort of keeping RWT and SWT in sync. Since some of the verbatim copies of SWT code used in RWT use parameter assignments setting the tooling flag on error would force a rewrite these passages to get rid of the errors (at least I don't know an other option). Maybe we can have a look at it and see how many errors arise. If it doesn't take too much effort and the SWT team is interested such changes could be recontributed to SWT? (In reply to comment #4) > We in Riena just a couple of month ago decided the opposite and converted all > our code. So now all variables (instant vars, local vars, parameters) are always > final unless they are modified in the code. I believe there is a setting for > that in the code and it is applied every time you save. And you cant assign > something to a parameter. I agree that it is a good practice never to change the value of a variable if you can avoid it. From this standpoint, it makes perfect sense to mark every variable as final. But as the final keywords also clutter the code, you have to make a trade-off between enforcing this rule and keeping you code readable. I'm sure that opinions will differ here. My feeling is that clarity outweighs the benefit of the final declarations by far. Please note that this is different to marking fields, classes, and methods as final. Making fields as final is necessary to be able to reason about mutable state. If all fields of a class are final, you can immediately see that this class is immutable. Final classes and methods can be necessary to prevent inheritance where it is not provided for. There is no comparable benefit in final keywords within method bodies or for parameters. Those only enforce a programming style, they do not affect the user of a class or method in any way. > It seems (from the outside) that you actually want this behaviour but dont want > the "final" word in your code and now you are working around this problem with > tooling. That's right, but I wouldn't call it a workaround. The tooling seems to be the right place to enforce coding style, while the compiler is the tool to enforce consistency. Finals are sometimes useful for anonymous class access. But not sure that this is a good example. ;) Frankly, all this work just for a small screen sounds a bit scary. Especially given the difference in nature between SWT and RWT. Frank, what about a larger screen? Ok, but seriously I don't mind removing final. I share Christian's opinion in general. They do less harm than removing them (IMO). But I just noticed that I don't have them in interfaces. Well, they don't make sense there. But it definitely is easier to read. (In reply to comment #9) > Frankly, all this work just for a small screen sounds a bit scary. I don't think that it is that much work using the cleanup functionality of eclipse, so IMHO its worth the effort. Rüdiger and I've been working for a while now without finals and a larger line length limit (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=338086) with very good results and every time we switch over to the RAP development workspace we feel that adapting these convention would help to make the code more readable. We had some discussion here but there were no real objections to getting rid of finals. I agree with Frank that this change will improve the readability of our codebase. There is also a consensus in the RAP team. I have changed the compiler settings of all relevant projects except rwt itself to flag parameter assignments as an error. The rwt bundle still reports them as a warning because of the SWT copies. I also opened bug 338080 for adding a code analysis tool to the build. Until then, we'll have to live with warnings in RWT - but I don't think that's a problem, because the odds of a parameter assignment slipping in unnoticed are rather low. I also adapted the Coding Conventions wiki page. (In reply to comment #7) > Maybe we can have a look at it and see how many errors arise. If it doesn't take > too much effort and the SWT team is interested such changes could be > recontributed to SWT? I checked this and we'd get almost 100 compile errors. Parameter assignments seem to be pretty common in the SWT coding style. Even if we could convince them, I guess it would only make sense to eliminate all parameter assignments in the entire SWT code base which seems to be quite an undertaking... But let's keep this in mind and talk the SWT guys at a suitable occasion. I would suggest to proceed as follows: Let's skip finals from now on when writing new code and remove them when working on existing code. At some point before the release, we can remove finals with some search and replace magic. But as this would break all existing patches, I would not like to do this right now. I think the remaining finals don't do any harm and we can live with this "mixed" code for some weeks. I'll keep this bug open to remind us of removing the remaining finals. I think I've removed most of the finals. Let's eliminate remaining ones according to the boyscout rule. |