Re: [xwiki-devs] Checkstyle audit audit

On 09/09/14 05:00, Caleb James DeLisle wrote: There's another checkstyle rule, which we don't use, preventing the use of numbers other than for defining constants. That is called "magic numbers", and the goal is the same as for duplicate strings: preventing the use of poor-man's constants sprinkled all over the code, with a hidden meaning defined in one hard to find place. The use of constants is supposed not just to make it easier to redefine that constant, but also to make it easy to find the place where the definition/meaning is. I think we already know not to use such constants in new code (the old core has many of those), and if anybody would accidentally use strings that way a code review will quickly find them, and then we can all publicly shame the poor developer that did that... True, most of the strings are 1-2 char constants or error messages. In light of this, I think it's pretty safe to disable this rule. You can send a vote. Many say that bad javadoc does more bad than good, since good code speaks for itself better than rotting comments can. However, my concern was that you have a @Role, which is supposed to be a public API, defined in an internal package. That's a private public interface... Which leads to point 3 below. You can send a vote for disabling javadoc requirements on internal classes, leaving it up to the developer to decide which classes/methods need an explanation. The problem is that there's no existing way of restricting a checkstyle rule to specific packages, so someone will have to extend the existing rule to ignore stuff in an internal package. And since you're the one complaining about the current rule, you should do that. The problem is that our component manager is severely handicapped by the fact that it can only see components, you can't @Inject a non-component into a component, or a component into a non-component. These use-cases are supposed to be supported by a JSR-299 compliant implementation. I'd say that there are 2 reasons for this interface-implementation split: 1. Forcing as much as possible the separation between the public contract (API) and the internal implementation details, even though there is only one obvious implementation. This is weak reason, IMHO, but it does respect good design patterns. 2. Allowing for future alternative implementations. Are you 100% sure there will always be only one possible implementation for that API? The XWiki Platform is a platform, not just the XWiki Enterprise, and we don't know what possible complex products others are building on top of that platform. I am one of those advanced integrators, and I do hit very often the limits of the APIs or their "default" implementations. Btw, the discussion about default implementations was: http://markmail.org/thread/mrbmbn45cltfvh57 followed by: http://markmail.org/thread/rzytq6j3vbsbtcb6 When we started writing our own component manager, JSR-299 was fairly new, and the existing implementations weren't that powerful. IIRC, what they lacked most was a way of defining a component just for a subwiki, and being able to install/remove components at runtime. Plus, the more powerful ones required a custom classloader, which isn't easy to use in a servlet container that we don't control. These are two critical requirements for our extension manager. I wonder if there's a library that has these features now, plus being able to work with non-components. The checkstyle isn't mandatory, you can add exceptions if you have a good reason to do that. The existing rules are supposed to make the code better, if you think they only make something worse, then add an exception, explain why you think the exception is needed, and smile :) -- Sergiu Dumitriu http://purl.org/net/sergiu