On 09/09/2014 03:22 PM, Sergiu Dumitriu wrote: > On 09/09/14 05:00, Caleb James DeLisle wrote: >> 1. Multiple identical string constants in a file > > 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.
This is a tricky decision. When developing cjdns I begin using 16 as a constant for memcpy of ipv6 addresses (they're certainly not going to change) but then I decided that was too much magic so I went to Address_TARGET_SIZE (which seemed to make sense since they're used as search targets). After some time went by, I started slipping back to using 16 and trying to read my old code with Address_KEY_SIZE and Address_TARGET_SIZE and so on hurt my brain so I ended up refactoring all of that back to 16 and 32 respectively. In this case I feel that my choice was good. memcpy(dest.ip6, srcBuffer, 16); is natural to read. Clearly pointer acrobatics with all various types of constants headerPtr += 32 + 24 + 4 + sizeof(struct somehdr); is horrifying, but I don't think one cannot derive a good algorithmic rule for knowing the difference. > > 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. Ok > >> 2. Javadoc /internal/ classes and methods > > 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. Being able to write free-form inside of a module makes it IMO worth it. Will do. > >> 3. (not a checkstyle rule) interfaces with one impl. > > 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. > >> 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. > > 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 :) > Well if we can take care of the first two, I'll have something to smile about :) Then maybe we can talk about requiring the use of 'this.' in field accesses. Thanks, Caleb _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
