[xwiki-devs] [RFC] Review of checkstyle rules.

Hi all, This is partially an extension of the previous mail thread ``Checkstyle audit audit'' where in the discussion, I formed a few thoughts about checkstyle rules which should be added and removed. Since that thread is very cluttered now, I wanted to begin a new one specifically explaining the changes which I think we should make. Comments are requested. All but the most trivial Codestyle rules all create friction which slows down work. They are however valuable because they identify likely mistakes and make it easier for others to read one's code. Still we must periodically review our rules and identify both new rules which can help us and existing rules which are more bother than they are help, either in general or in specific to the experience level of our team. Changes I would like to make: #1: Remove "duplicate strings" checkstyle rule. This rule is supposed to trap people who are hardcoding constants throughout their code instead of defining them in one place. However when writing this email, I tried to find a coherent example which is much improved by hoisting the string value up into a constant and I cannot think of a single example. False positive examples spring to mind immediately, usually log/error messages or on strings which are sure never to change (at least not before the next major refactoring). "fixing" these errors makes the code worse because one can no-longer read top to bottom, one must scroll up and down between constants and code. I think this rule is more harm than good, especially for our team's experience level. #2: Set class/method/field javadoc requirement to exempt private and package private entities. When I write code, I want to do everything the easiest way that could work. When I read code, I want every comment to be giving me important and up-to-date information. If checkstyle makes me write comments which teach programming, I'm going to be mad at checkstyle, if I try to debug your code and the important details/warnings are mixed in with a rash of CS-101 babble, I'm going to be mad at you. In either case it takes me longer to do what I set out to do. The solution of requiring javadoc only on public classes/interfaces also helps me when developing because I need to think more deeply about the interfaces which I am exposing to the world. "write javadoc here" or "this needs an @param tag" implies "hey, did you really intend to make that public API?". I think the change not only lifts the burden on the programmer but also actually helps checkstyle achieve it's goals more efficiently. #3: Add rule requiring use of 'this.' when accessing class fields and remove rule preventing variable names which shadow fields. This change makes checkstyle *more* strict but I believe the cost is reasonable and the benefit is high. Reading your code I immediately know whether you're accessing a field or a variable which tells me whether there is reason why your function might behave differently depending on the state of the object. In light of this change, there is nolonger any reason to concern ourselves with shadowing of field names since we cannot access them this way so I think the new rule should be accompanied by the removal of the shadowing rule. I'd like to hear comments on these proposed changes. Thanks, Caleb