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 > > -- > Sergiu Dumitriu > http://purl.org/net/sergiu > _______________________________________________ > devs mailing list > devs(a)xwiki.org > http://lists.xwiki.org/mailman/listinfo/devs