Re: [xwiki-devs] Checkstyle audit audit

Hi, My own humble remarks ... :) 2014-09-09 11:00 GMT+02:00 Caleb James DeLisle <cjd(a)cjdns.fr>fr>: There's the implicit idea here that a rule is something created to bother developers. Something that limit contribution, is costly eventually, requires an effort, and so on ... I think the concept and objectives of such rules is exactly the contrary, they're here to help contribution (by making the code clearer), and give a framework that everyone knows and understands (less effort to dig into code you didn't write). Of course there are always rules that can be discussed, regarding their pertinence, criticality etc. The issue I see here, is answering the question : "only one implementation will ever make sense". Other contributors may invent over implementations you never thought about. Following IOC and java interfaces concepts, any component should have a contract - even if there will be only one, ever. Question is more if you are willing to allow this code to be replaced by another implementation or not, more than if it makes sense or not at the time being, IMHO, because you don't want thousands of components everywhere obviously. If I want to know what's the return value I will only check the return description in javadoc, maybe not the sometimes length full description above of it ... As much as I love xwiki, I must say I'm not a bit fan of old core javadoc ;-) Take these ones for example: http://nexus.xwiki.org/nexus/service/local/repositories/releases/archive/or… http://nexus.xwiki.org/nexus/service/local/repositories/releases/archive/or… I dare any contributor to find what they do without looking at the code :) All this discussion is a proof that it was actually needed to comment this ! :) On my side if I just see "XWikiWebSocketScriptService#getUser", I have absolutely no idea of what it could return ... - current user ? - user of the web socket ? (if it means something) - user that owns a thread of the web socket service ? ... IMHO something called "getXxx" or "setXxx" is implicitely described only in case of a bean or pojo, in a script service or component interface there's nothing really obvious usually ... I think what's really missing here is that they never really exposed an URL separator constant in the JDK ... But it's difficult for a checking-tool to differentiate between those url separators (that will never change, so who cares as you say), and using file paths separators hard-coded (which is pretty bad and create many environment specific weirdness in so many tools). I think if they did that with maven plugins from the start, they wouldn't be hunting for these '/' and '\' like they do, and they'd have lighten many JIRAs ... Default scope for javadocs with maven plugin is protected ... Not public. Personnally I (try) to put javadocs everywhere, even on private methods (Hey, I don't succeed, you can blame me ;) ) Because I describe there the contract of the method - not how it's implemented - there are non-javadoc comments for that. And often I'm the first who reads them, when I'm back to work after a long time : "so, what was this for again ... ?" Sometimes the name of the method is obvious, but almost all the time it's not sufficient. It's also a nice way to document a change - you see the change in contract, and impact on implementation, or you see that implementation changed, but not the contract. (when I say contract here it's more than the method signature) But that's mainly my point of view because I like javadocs ! :) Personnally I don't like those kind of external links, because in some time from now they could easily have disappeared, leading to broken links everywhere in your javadoc ... I'd document these in the overall component/module/software documentation, in a wiki, in somewhere easily up-to-date. I think the fix should logically bring another broken rule about appending strings with '+' :D I'm not sure where should be put the cursor, from "it makes development harder" compared to "it's an important rule", apart from applying rules that were agreed among a community ... If I ever perform a modification on this code, let's say around the first RuntimeException, and I changed the name of the property to something else than "websocket.ssl.blabla", I could maybe easily miss the fact that this string appears somewhere else, then the user would get a different property name in different exceptions depending on the use-case, one good one wrong. (blame me for being a bad developer not searching for this string in all the code if you want :), but remember, I'm a contributor, not a committer, you didn't hire me on my skills, you can just provide me as many recommendations as you want, and I may never follow them. Your only possibility is to review my code thoroughly (which will not improve time for merging contributed code), or rely on some tool to automate some basic checks.) BTW IMO it should be the name of this property that should be in a constant, even before this exception message string. I mostly agree with you on this. My personal idea is that there should be a limit, but far greater than the usual or expected limit. But maybe overriding the rule is enough on a case per case basis ... In my case I usually write much more statements than most developers would do, because I can't do another way :) I prefer to write all little steps, and let the compiler do its job, and it's the way I think it. I think my code would most of the time break this rule ... :/ As any rule has exceptions, there are exceptions to the compelling justification for a rule ... Maybe some rules are bad, or badly specified, or there's just some rare cases where they're more annoying than useful, but would you disable them completely in all cases ?