Re: [xwiki-devs] Checkstyle audit audit

Firstly, thank you very much for reviewing my code. When I was young I hated homework, my parents told me that if I spent as much time on the work as I spent bellyaching about it, I'd have it done already. A lot of your review seems to take the point of view that it's "not that hard", my counter is that no rule makes contributing *easier* and as contributors are not exactly lining up at the door, any rule that exists needs to justify itself as necessary. On 09/08/2014 07:30 PM, Sergiu Dumitriu wrote: This is the first time I've heard that. Indeed I don't like "Default" either but it seems we don't use "Impl". This question brings up a more difficult point, why do we need an interface for each thing we register in the CM, even if there's only ever going to be one impl? Although I'm tempted now to copy/paste your comment into the code, I'm not hesitant because this is not the XWiki Configuration infra, just one module's config. If we hardcoded "xwiki.properties" all over the code, that would be well accepted as a disaster, why then is it ok to hardcode documentation about how XWiki's configuration works all over the codebase? This class uses ConfigurationSource, whatever that might do. Business logic in the configuration? Should the configuration also be responsible for checking if the file exists? Skirting the question.

Do you really think that a 3 line function called somethingOtherThanGetUser() needs a javadoc comment?

I do want a reference to XWikiGuest, null has a different meaning in this context. There are of course many ways to solve a given problem but if you claim that the implementation detail of how the internals of this class behave are wrong and deprecated, the burden of proof is on you. That's a good point, I'll document those. >> I can guarantee you that the competition are not going to slow down and smell the flowers. >> >> >> /src/main/java/org.xwiki.contrib.websocket/internal/XWikiWebSocketScriptService.java:121:79: The String "/" appears 4 times in the file. >> >> And it might appear 400 times, defining a static final string called URL_SEPERATOR is >> exercise for my fingers and your eyeballs, nothing more. If I really wanted to obfuscate >> my code, I could use an external library to make you go hunting for the value of >> YouWillNeverFindMe.SEPERATOR > > We can exclude certain strings from the check. We currently exclude a > few common strings: "", "[0-9]", " ", "]". We could add "/" as well. > > This rule is supposed to protect us against poor-man's constants > copy-pasted in many places, since this is error-prone (typos) and hard > to refactor. It also forces us to define useless constants just for > working around this rule, which I agree is almost as bad. There's a > tradeoff between maintainability and convenience, and the XWiki > community agreed to go for more maintainability.

It's not more maintainable, it just looks more maintainable. If someone's hardcoding filesystem paths inside of classes then they're certainly doing so many other horrible things that the file paths should be preserved as a red flag for reviewers! 99% of strings are not file paths, they're error messages and forward slashes and other things which will never change. Even configuration keys are impossible to change once they're deployed in the field. #define TEN 10 Because it's internal, I assume that you can just look at the implementation. But this gets back to the question of why we need to define interfaces for which there will never be more than one impl. Override indeed solves the checkstyle error. Does telling the reader that Netty means netty.io make the code fundamentally better? If so then I could make all of my code better by telling the reader how to use google. Easy fix is still worse than "not broken", I don't want to know how easy it is to trick this rule, I know a lot of tricks already. I want to hear a justification for this rule in the face of the fact that it makes development harder than it would otherwise be. >> /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:118:92: '+' should be on a new line. >> /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:122:92: '+' should be on a new line. >> /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:132:94: '+' should be on a new line. >> /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:133:92: '+' should be on a new line. >> >> It's arguably better to put it on the line before because some languages (python) >> infer semicolons and will fail to parse if it's on the line after. >

> This isn't Python code. But I agree, I don't have a good reason for > putting it on the next line, and I've wondered about this rule as well.

I write code in a number of languages and I want to adopt habits which are as widely applicable as possible. I've written functions much longer and much easier to read as well as much shorter functions which, due to the complexity of the algorithm, push my own mental capacity to it's limit. Surely I still have much room for improvement in code craftsmanship but I don't think a checkstyle failure is any more likely to make me suddenly discover a clearer way to express a function any more than a rejected paper would suddenly make me a mathematician.

On the contrary, fixing these types of errors will almost certainly make the code *worse* because the original state of the function is an expression of the mental model of the author and in response to failures, he will break it up along arbitrary lines in order to make the errors stop, but the code and the programmer will not and cannot be fundamentally better than they were 20 minutes before. I don't want to see the tricks, I want to see compelling justification for the rule. If I gave you a string and a handful of URI manipulating functions then even if you have no knowledge of those functions, you can still understand it as a plain old string, if I give you a java.net.URI then you have to go look up API documentation no matter what, an obvious cost increase. If there are no solid justifications for this cost then I call overengineering. It's not enforced by checkstyle, ftr this is one rule which I would like to see added (but only if we can at least balance the development pain budget, if not lower it). It will "probably never happen" but I fixed it for the next version, thanks for spotting that. Since it's used in an inner class, I though I needed it to be final but indeed using the field works, fixed. So to summarize, most painful rules: 1. Multiple identical string constants in a file 2. Javadoc /internal/ classes and methods 3. (not a checkstyle rule) interfaces with one impl. 4. There are 3 or 4 rules which force you to completely re-factor a function or class, every time one of these trips and the developer "fixes it", we're creating bad code. Thanks, Caleb _______________________________________________ devs mailing list devs(a)xwiki.org http://lists.xwiki.org/mailman/listinfo/devs