[xwiki-devs] Checkstyle audit audit

Hi, I just finished rewriting my WebSocket extension using Netty and it now supports SSL. \:D/ To test my work before re-releasing I decided to enable checkstyle and see what issues it pointed out which were important for me to fix. It listed 78 checkstyle errors, many of them are very important and helpful to me, others are second guessing my ability to write clear code or even pedantically pointing out issues of little or no importance. While I was sifting through them, I thought I'd write a little review of our checkstyle rules as seen from my eyes. /src/main/java/org.xwiki.contrib.websocket/WebSocketHandler.java:20:9: Package declaration does not match directory 'org/xwiki/contrib/websocket'. This is possibly good because it will catch errors, in my case it stopped me because I used one directory called org.xwiki.contrib.websocket as opposed to nested org/xwiki/contrib/websocket directories. Perhaps either form should be allowed but it's still useful because it will trap accidental moving of classes from a different directory. Why can't javac infer package from directory?! :) /src/main/java/org.xwiki.contrib.websocket/WebSocketHandler.java:24: Missing a Javadoc comment. /src/main/java/org.xwiki.contrib.websocket/WebSocketHandler.java:27:5: Missing a Javadoc comment. /src/main/java/org.xwiki.contrib.websocket/WebSocketHandler.java:27:5: Redundant 'public' modifier. This is a non-internal interface so it's valid that there should be *some* javadoc. /src/main/java/org.xwiki.contrib.websocket/internal/EchoWebSocketHandler.java:41: Line matches the illegal pattern 'System\.(out|err)\.'. Thank you, I forgot all about that one :) /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. /src/main/java/org.xwiki.contrib.websocket/internal/WebSocketConfig.java:24: Missing a Javadoc comment. Documentation on an interface, same as above. /src/main/java/org.xwiki.contrib.websocket/internal/WebSocketConfig.java:28: Expected an @return tag. /src/main/java/org.xwiki.contrib.websocket/internal/WebSocketConfig.java:34: Expected an @return tag. /src/main/java/org.xwiki.contrib.websocket/internal/WebSocketConfig.java:37: Expected an @return tag. /src/main/java/org.xwiki.contrib.websocket/internal/WebSocketConfig.java:40: Expected an @return tag. /src/main/java/org.xwiki.contrib.websocket/internal/WebSocketConfig.java:52: Expected an @return tag. /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? /src/main/java/org.xwiki.contrib.websocket/internal/XWikiWebSocketScriptService.java:35:1: Redundant import from the same package - org.xwiki.contrib.websocket.internal.WebSocketService. Thankyou! This is very helpful, fixed. /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 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 /src/main/java/org.xwiki.contrib.websocket/internal/XWikiWebSocketScriptService.java:136:16: 'catch' is not followed by whitespace. Thanks, fixed. /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. /src/main/java/org.xwiki.contrib.websocket/internal/WebSocketService.java:28:5: Redundant 'public' modifier. fixed /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. /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:24:8: Unused import - java.io.StringWriter. /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:29:8: Unused import - javax.inject.Provider. /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:31:8: Unused import - java.io.IOException. /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:34:8: Unused import - org.apache.commons.io.IOUtils. /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:44:1: Duplicate import to line 36 - org.apache.commons.lang3.exception.ExceptionUtils. This is what I'm here for, these are one of the most valuable pieces of feedback. /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:85: Missing a Javadoc comment. /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:115:5: Missing a Javadoc comment. /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. /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. /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:138:5: Missing a Javadoc comment. /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:176:5: Missing a Javadoc comment. /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. /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. /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:246:64: '+' is not preceded with whitespace. /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:246:65: '+' is not followed by whitespace. Ok /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:267: method call child at indentation level 23 not at correct indentation, 24 Ok /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:278:41: Empty catch block. Ok /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. /src/main/java/org.xwiki.contrib.websocket/WebSocket.java:27: Type Javadoc comment is missing an @version tag. My name is not git, I just am one. Thanks, Caleb