Re: [xwiki-devs] Checkstyle audit audit

I'm gonna audit your audit audit. On 09/08/2014 05:14 AM, Caleb James DeLisle wrote: Once again, I don't like "DefaultXYZ" as a name for components. Either only one implementation will ever make sense, in which case it probably shouldn't be an interface+implementation, or it would make sense to have alternate implementations. In the latter case, there's nothing "default" about the implementation, other than it is used in the XWiki Platform as the default implementation. The name should mention what's specific to the implementation, and so should the javadoc: /** Implementation for the {@link WebSocketConfig} role which allows configuring the {@link XWikiWebSocketScriptService websocket service} through the usual XWiki configuration places: space preferences, wiki preferences, and {@code xwiki.properties}. */ public class WikiWebSocketConfig I usually describe the meaning of the return value in the method javadoc, and the format / possible values / special cases in the @return javadoc: /** * The filename of the SSL private key in OpenSSL PEM format. * * @return an absolute path, or {@code null} if no certificate is * configured */ String getPrivateKeyFilename(); (Why doesn't this return a File, or an InputStream, since that's safer than a raw path?) I think that getUser doesn't belong here at all. Since it just returns the current user, why is it needed? There's already $xcontext.userReference, and if you want to actually return a reference to XWikiGuest instead of null, then that's wrong and deprecated. As for the other longer methods, since this is a public API it should be documented. Also, it was agreed that script services should be in a non-internal org.xwiki.module.script package. 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 a component role, but it's internal. That's a bit of a contradiction, but let's ignore that for the moment. I looked at that interface (ignoring the JavaDoc that's there now), and out of context I can't infer it's meaning. Except the type, these actually are missing an @Override. As for the type: /** * {@link WebSocket} implementation using the * <a href="http://netty.io/">Netty library</a>. */ Easy fix: - throw new RuntimeException("SSL enabled with websocket.ssl.certChainFile set " + - "but the pkcs8PrivateKeyFile does not seem to exist."); + throw new RuntimeException( + "SSL enabled with websocket.ssl.certChainFile set but the pkcs8PrivateKeyFile does not seem to exist."); 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. Would you say the same if the limit was at 5000 statements, and you were over that limit? We have to draw the line somewhere, and it was randomly agreed to set it at 30. This might be too low for some usecases, but in general it is a good enough rule IMHO. You can use your judgment and explicitly disable that rule for that function. But looking at the code, I think it could use some simplifying, I find it rather long. The rule isn't just about code complexity, but the attention span of a developer reading that code. How long does it take to put that whole method in the brain? Can most developers even fit that much code in memory? If not, then the method must be simplified / split. String key = StringUtils.substringAfter(uri, "?k="); uri = StringUtils.substringBefore(uri, "?"); (Why would any serious library use strings for representing URIs instead of java.net.URI?) That's not an absence, the "always use this." rule is there. Having both rules is a bit redundant, since the this rule is supposed to allow the field hiding that the second rule is complaining about. Still, why do you need to declare nws instead of assigning to this.nws? As I can see, there's no exception that could stop the method between the nwsLocal declaration and the this.nws assignment. And I think you just introduced a bug, https://github.com/xwiki-contrib/xwiki-contrib-websocket/blob/master/src/ma… now references "nws" which became this.nws instead of nwsLocal. -- Sergiu Dumitriu http://purl.org/net/sergiu