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

Hi, I haven’t had time to read this thread yet. Very generally: - checkstyle is a dead tool - the sonar guys have been reimplementing the rules in a better way - sonar offers more rules (pmd, findbugs, etc) - I’ve started defining rules we may want to use in this email thread: http://xwiki.markmail.org/thread/vnggoomrbqbfaojn - I’ve already excluded rules, listed on http://design.xwiki.org/xwiki/bin/view/Proposal/DefineSonarRules - One next step is to look at http://sonar.xwiki.org to see the remaining violations and decide which one we want to remove - Another step will be to decide to use the sonar maven plugin to act as a replacement for the checkstyle maven plugin and fail the build in case of errors - The list of currently enabled rules are available here: http://sonar.xwiki.org/coding_rules#qprofile=java-xwiki-57409|activation=tr… Thanks -Vincent On 12 Sep 2014 at 18:27:50, Sergiu Dumitriu (sergiu@xwiki.org(mailto:sergiu@xwiki.org)) wrote: > I've been working on and off on updating our usage of Checkstyle, since > we haven't really updated our checkstyle configuration since we first > started using it, other than minor tweaks. In the latest version of the > Checkstyle tool several useful checks have been added, which I'd like to > enable. > > On 09/12/2014 04:15 AM, Caleb James DeLisle wrote: >> 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. > > +0 on this one, it's an extra safety net, but one that I agree is a bit > too cumbersome, and I'm not a big fan of defining private constants just > to work around this rule. > >> #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. > > +1. All public code should be thoroughly documented, and private code > should only document what's not obvious. > >> #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. > > +1, but there's currently a bug in that rule: > https://github.com/checkstyle/checkstyle/pull/274 > > More rules to enable in https://github.com/xwiki/xwiki-commons/pull/9