I'm gonna audit your audit audit. On 09/08/2014 05:14 AM, Caleb James DeLisle wrote:
/src/main/java/org.xwiki.contrib.websocket/internal/DefaultWebSocketConfig.java:26: Missing a Javadoc comment. Missing javadoc on a class... whose behavior is very obvious. I'm not here to teach programming.
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
/src/main/java/org.xwiki.contrib.websocket/internal/WebSocketConfig.java:55: Expected an @return tag.
I actually wrote javadoc on all of these, without even being prodded to do so. Do you think my freeform documentation is less useful to a reader than it would be if it contained a @return tag?
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?)
/src/main/java/org.xwiki.contrib.websocket/internal/XWikiWebSocketScriptService.java:45: Missing a Javadoc comment. /src/main/java/org.xwiki.contrib.websocket/internal/XWikiWebSocketScriptService.java:50:5: Missing a Javadoc comment. /src/main/java/org.xwiki.contrib.websocket/internal/XWikiWebSocketScriptService.java:100:5: Missing a Javadoc comment.
Do you really think that a 3 line function called getUser() needs a javadoc comment?
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.
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.
/src/main/java/org.xwiki.contrib.websocket/internal/WebSocketService.java:25: Missing a Javadoc comment. /src/main/java/org.xwiki.contrib.websocket/internal/WebSocketService.java:28:5: Missing a Javadoc comment.
It's inside of an /internal/ directory, if you can't infer it's meaning then either you can't read or I can't code.
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.
/src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocket.java:33: Missing a Javadoc comment. /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocket.java:58:5: Missing a Javadoc comment. /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocket.java:59:5: Missing a Javadoc comment. /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocket.java:60:5: Missing a Javadoc comment. /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocket.java:62:5: Missing a Javadoc comment. /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocket.java:67:5: Missing a Javadoc comment. /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocket.java:69:5: Missing a Javadoc comment. /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocket.java:70:5: Missing a Javadoc comment.
It's internal and this module is small, if a reasonable programmer needs this granularity to understand this module, it should be rewritten from scratch.
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>. */
/src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:118:40: The String "SSL enabled with websocket.ssl.certChainFile set " appears 2 times in the file.
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.");
/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.
/src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:235:9: Executable statement count is 37 (max allowed is 30).
Statements per function is almost useless as a measure of code complexity. I claim negative usefulness because it is misleading.
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.
/src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:246:58: The String "?k=" appears 2 times in the file.
And you don't need to look anything up to understand that line of code, any change would be for the worse.
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?)
/src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:300:38: 'nws' hides a field.
I always use 'this.' prefix when accessing object fields, I would prefer that as a rule but in the absence of that rule, this one is fairly important.
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/mai... now references "nws" which became this.nws instead of nwsLocal. -- Sergiu Dumitriu http://purl.org/net/sergiu