[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
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
Firstly, thank you very much for reviewing my code. When I was young I hated homework, my parents told me that if I spent as much time on the work as I spent bellyaching about it, I'd have it done already. A lot of your review seems to take the point of view that it's "not that hard", my counter is that no rule makes contributing *easier* and as contributors are not exactly lining up at the door, any rule that exists needs to justify itself as necessary. On 09/08/2014 07:30 PM, Sergiu Dumitriu wrote:
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"
This is the first time I've heard that. Indeed I don't like "Default" either but it seems we don't use "Impl". This question brings up a more difficult point, why do we need an interface for each thing we register in the CM, even if there's only ever going to be one impl?
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
Although I'm tempted now to copy/paste your comment into the code, I'm not hesitant because this is not the XWiki Configuration infra, just one module's config. If we hardcoded "xwiki.properties" all over the code, that would be well accepted as a disaster, why then is it ok to hardcode documentation about how XWiki's configuration works all over the codebase? This class uses ConfigurationSource, whatever that might do.
/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?)
Business logic in the configuration? Should the configuration also be responsible for checking if the file exists?
/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
Skirting the question. Do you really think that a 3 line function called somethingOtherThanGetUser() needs a javadoc comment?
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.
I do want a reference to XWikiGuest, null has a different meaning in this context. There are of course many ways to solve a given problem but if you claim that the implementation detail of how the internals of this class behave are wrong and deprecated, the burden of proof is on you.
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.
That's a good point, I'll document those.
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.
It's not more maintainable, it just looks more maintainable. If someone's hardcoding filesystem paths inside of classes then they're certainly doing so many other horrible things that the file paths should be preserved as a red flag for reviewers! 99% of strings are not file paths, they're error messages and forward slashes and other things which will never change. Even configuration keys are impossible to change once they're deployed in the field. #define TEN 10
/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.
Because it's internal, I assume that you can just look at the implementation. But this gets back to the question of why we need to define interfaces for which there will never be more than one impl.
/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:
Override indeed solves the checkstyle error.
/** * {@link WebSocket} implementation using the * <a href="http://netty.io/">Netty library</a>. */
Does telling the reader that Netty means netty.io make the code fundamentally better? If so then I could make all of my code better by telling the reader how to use google.
/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.");
Easy fix is still worse than "not broken", I don't want to know how easy it is to trick this rule, I know a lot of tricks already. I want to hear a justification for this rule in the face of the fact that it makes development harder than it would otherwise be.
/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.
I write code in a number of languages and I want to adopt habits which are as widely applicable as possible.
/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.
I've written functions much longer and much easier to read as well as much shorter functions which, due to the complexity of the algorithm, push my own mental capacity to it's limit. Surely I still have much room for improvement in code craftsmanship but I don't think a checkstyle failure is any more likely to make me suddenly discover a clearer way to express a function any more than a rejected paper would suddenly make me a mathematician. On the contrary, fixing these types of errors will almost certainly make the code *worse* because the original state of the function is an expression of the mental model of the author and in response to failures, he will break it up along arbitrary lines in order to make the errors stop, but the code and the programmer will not and cannot be fundamentally better than they were 20 minutes before.
/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, "?");
I don't want to see the tricks, I want to see compelling justification for the rule.
(Why would any serious library use strings for representing URIs instead of java.net.URI?)
If I gave you a string and a handful of URI manipulating functions then even if you have no knowledge of those functions, you can still understand it as a plain old string, if I give you a java.net.URI then you have to go look up API documentation no matter what, an obvious cost increase. If there are no solid justifications for this cost then I call overengineering.
/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.
It's not enforced by checkstyle, ftr this is one rule which I would like to see added (but only if we can at least balance the development pain budget, if not lower it).
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.
It will "probably never happen" but I fixed it for the next version, thanks for spotting that. Since it's used in an inner class, I though I needed it to be final but indeed using the field works, fixed. So to summarize, most painful rules: 1. Multiple identical string constants in a file 2. Javadoc /internal/ classes and methods 3. (not a checkstyle rule) interfaces with one impl. 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. Thanks, Caleb
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. 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.
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.
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 :) -- Sergiu Dumitriu http://purl.org/net/sergiu
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
Hi, My own humble remarks ... :) 2014-09-09 11:00 GMT+02:00 Caleb James DeLisle <[email protected]>:
Firstly, thank you very much for reviewing my code.
When I was young I hated homework, my parents told me that if I spent as much time on the work as I spent bellyaching about it, I'd have it done already. A lot of your review seems to take the point of view that it's "not that hard", my counter is that no rule makes contributing *easier* and as contributors are not exactly lining up at the door, any rule that exists needs to justify itself as necessary.
There's the implicit idea here that a rule is something created to bother developers. Something that limit contribution, is costly eventually, requires an effort, and so on ... I think the concept and objectives of such rules is exactly the contrary, they're here to help contribution (by making the code clearer), and give a framework that everyone knows and understands (less effort to dig into code you didn't write). Of course there are always rules that can be discussed, regarding their pertinence, criticality etc.
On 09/08/2014 07:30 PM, Sergiu Dumitriu wrote:
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"
This is the first time I've heard that. Indeed I don't like "Default" either but it seems we don't use "Impl". This question brings up a more difficult point, why do we need an interface for each thing we register in the CM, even if there's only ever going to be one impl?
The issue I see here, is answering the question : "only one implementation will ever make sense". Other contributors may invent over implementations you never thought about. Following IOC and java interfaces concepts, any component should have a contract - even if there will be only one, ever. Question is more if you are willing to allow this code to be replaced by another implementation or not, more than if it makes sense or not at the time being, IMHO, because you don't want thousands of components everywhere obviously.
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
Although I'm tempted now to copy/paste your comment into the code, I'm not hesitant because this is not the XWiki Configuration infra, just one module's config.
If we hardcoded "xwiki.properties" all over the code, that would be well accepted as a disaster, why then is it ok to hardcode documentation about how XWiki's configuration works all over the codebase?
This class uses ConfigurationSource, whatever that might do.
/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?
If I want to know what's the return value I will only check the return description in javadoc, maybe not the sometimes length full description above of it ... As much as I love xwiki, I must say I'm not a bit fan of old core javadoc ;-) Take these ones for example: http://nexus.xwiki.org/nexus/service/local/repositories/releases/archive/org...) http://nexus.xwiki.org/nexus/service/local/repositories/releases/archive/org...) I dare any contributor to find what they do without looking at the code :)
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?)
Business logic in the configuration? Should the configuration also be responsible for checking if the file exists?
/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
Skirting the question. Do you really think that a 3 line function called somethingOtherThanGetUser() needs a javadoc comment?
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.
I do want a reference to XWikiGuest, null has a different meaning in this context. There are of course many ways to solve a given problem but if you claim that the implementation detail of how the internals of this class behave are wrong and deprecated, the burden of proof is on you.
All this discussion is a proof that it was actually needed to comment this ! :) On my side if I just see "XWikiWebSocketScriptService#getUser", I have absolutely no idea of what it could return ... - current user ? - user of the web socket ? (if it means something) - user that owns a thread of the web socket service ? ... IMHO something called "getXxx" or "setXxx" is implicitely described only in case of a bean or pojo, in a script service or component interface there's nothing really obvious usually ...
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.
That's a good point, I'll document those.
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.
I think what's really missing here is that they never really exposed an URL separator constant in the JDK ... But it's difficult for a checking-tool to differentiate between those url separators (that will never change, so who cares as you say), and using file paths separators hard-coded (which is pretty bad and create many environment specific weirdness in so many tools). I think if they did that with maven plugins from the start, they wouldn't be hunting for these '/' and '\' like they do, and they'd have lighten many JIRAs ...
It's not more maintainable, it just looks more maintainable. If someone's hardcoding filesystem paths inside of classes then they're certainly doing so many other horrible things that the file paths should be preserved as a red flag for reviewers!
99% of strings are not file paths, they're error messages and forward slashes and other things which will never change. Even configuration keys are impossible to change once they're deployed in the field.
#define TEN 10
/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.
Because it's internal, I assume that you can just look at the implementation. But this gets back to the question of why we need to define interfaces for which there will never be more than one impl.
Default scope for javadocs with maven plugin is protected ... Not public. Personnally I (try) to put javadocs everywhere, even on private methods (Hey, I don't succeed, you can blame me ;) ) Because I describe there the contract of the method - not how it's implemented - there are non-javadoc comments for that. And often I'm the first who reads them, when I'm back to work after a long time : "so, what was this for again ... ?" Sometimes the name of the method is obvious, but almost all the time it's not sufficient. It's also a nice way to document a change - you see the change in contract, and impact on implementation, or you see that implementation changed, but not the contract. (when I say contract here it's more than the method signature) But that's mainly my point of view because I like javadocs ! :)
/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:
Override indeed solves the checkstyle error.
/** * {@link WebSocket} implementation using the * <a href="http://netty.io/">Netty library</a>. */
Does telling the reader that Netty means netty.io make the code fundamentally better? If so then I could make all of my code better by telling the reader how to use google.
Personnally I don't like those kind of external links, because in some time from now they could easily have disappeared, leading to broken links everywhere in your javadoc ... I'd document these in the overall component/module/software documentation, in a wiki, in somewhere easily up-to-date.
/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.");
Easy fix is still worse than "not broken", I don't want to know how easy it is to trick this rule, I know a lot of tricks already. I want to hear a justification for this rule in the face of the fact that it makes development harder than it would otherwise be.
I think the fix should logically bring another broken rule about appending strings with '+' :D I'm not sure where should be put the cursor, from "it makes development harder" compared to "it's an important rule", apart from applying rules that were agreed among a community ... If I ever perform a modification on this code, let's say around the first RuntimeException, and I changed the name of the property to something else than "websocket.ssl.blabla", I could maybe easily miss the fact that this string appears somewhere else, then the user would get a different property name in different exceptions depending on the use-case, one good one wrong. (blame me for being a bad developer not searching for this string in all the code if you want :), but remember, I'm a contributor, not a committer, you didn't hire me on my skills, you can just provide me as many recommendations as you want, and I may never follow them. Your only possibility is to review my code thoroughly (which will not improve time for merging contributed code), or rely on some tool to automate some basic checks.) BTW IMO it should be the name of this property that should be in a constant, even before this exception message string.
/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.
I write code in a number of languages and I want to adopt habits which are as widely applicable as possible.
/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.
I've written functions much longer and much easier to read as well as much shorter functions which, due to the complexity of the algorithm, push my own mental capacity to it's limit.
Surely I still have much room for improvement in code craftsmanship but I don't think a checkstyle failure is any more likely to make me suddenly discover a clearer way to express a function any more than a rejected paper would suddenly make me a mathematician.
On the contrary, fixing these types of errors will almost certainly make the code *worse* because the original state of the function is an expression of the mental model of the author and in response to failures, he will break it up along arbitrary lines in order to make the errors stop, but the code and the programmer will not and cannot be fundamentally better than they were 20 minutes before.
I mostly agree with you on this. My personal idea is that there should be a limit, but far greater than the usual or expected limit. But maybe overriding the rule is enough on a case per case basis ... In my case I usually write much more statements than most developers would do, because I can't do another way :) I prefer to write all little steps, and let the compiler do its job, and it's the way I think it. I think my code would most of the time break this rule ... :/
/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, "?");
I don't want to see the tricks, I want to see compelling justification for the rule.
As any rule has exceptions, there are exceptions to the compelling justification for a rule ... Maybe some rules are bad, or badly specified, or there's just some rare cases where they're more annoying than useful, but would you disable them completely in all cases ?
(Why would any serious library use strings for representing URIs instead of java.net.URI?)
If I gave you a string and a handful of URI manipulating functions then even if you have no knowledge of those functions, you can still understand it as a plain old string, if I give you a java.net.URI then you have to go look up API documentation no matter what, an obvious cost increase.
If there are no solid justifications for this cost then I call overengineering.
/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.
It's not enforced by checkstyle, ftr this is one rule which I would like to see added (but only if we can at least balance the development pain budget, if not lower it).
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.
It will "probably never happen" but I fixed it for the next version, thanks for spotting that. Since it's used in an inner class, I though I needed it to be final but indeed using the field works, fixed.
So to summarize, most painful rules:
1. Multiple identical string constants in a file 2. Javadoc /internal/ classes and methods 3. (not a checkstyle rule) interfaces with one impl. 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.
Thanks, Caleb _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
Hi Jeremie, Sorry for not replying sooner, I felt that I should give your mail a bit more thought and a little but more background about me since we don't know eachother as well as I know Sergiu and Marius. On 09/09/2014 06:02 PM, Jeremie BOUSQUET wrote:
Hi,
My own humble remarks ... :)
2014-09-09 11:00 GMT+02:00 Caleb James DeLisle <[email protected]>:
Firstly, thank you very much for reviewing my code.
When I was young I hated homework, my parents told me that if I spent as much time on the work as I spent bellyaching about it, I'd have it done already. A lot of your review seems to take the point of view that it's "not that hard", my counter is that no rule makes contributing *easier* and as contributors are not exactly lining up at the door, any rule that exists needs to justify itself as necessary.
There's the implicit idea here that a rule is something created to bother developers. Something that limit contribution, is costly eventually, requires an effort, and so on ... I think the concept and objectives of such rules is exactly the contrary, they're here to help contribution (by making the code clearer), and give a framework that everyone knows and understands (less effort to dig into code you didn't write).
Although in this mail thread I have been very critical, I should probably show my credentials as "pro best practice". I'm a leader of a project in C and I developed my own codestyle validator which checks, among other things, that all publicly accessible functions are prefixed with the name of the file where they're defined. Although I have on occasion wanted to violate this rule, I have yet to find what I think is a *valid* reason for doing so and I have so far kept it 100% which makes reading the code much easier for other people who upon seeing a function which they don't recognize, need only scroll to the top of the file to see which header it's defined in.
Of course there are always rules that can be discussed, regarding their pertinence, criticality etc.
That's what I'm after, I feel that every rule imparts a certain amount of cost and we need to periodically review them to make sure that they are net-positive value.
On 09/08/2014 07:30 PM, Sergiu Dumitriu wrote:
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"
This is the first time I've heard that. Indeed I don't like "Default" either but it seems we don't use "Impl". This question brings up a more difficult point, why do we need an interface for each thing we register in the CM, even if there's only ever going to be one impl?
The issue I see here, is answering the question : "only one implementation will ever make sense". Other contributors may invent over implementations you never thought about.
I've been here myself, I have made a lot of very nice interfaces to "future proof" my designs, and then when a new use case actually came along, I found my interfaces unduely difficult to use for that use case and I promptly re-factored the code. Since then, I've developed a strong tendency toward YAGNI and "simplest thing that could possibly work". By the time someone finds a new use case, the code probably needs some refactoring anyway.
Following IOC and java interfaces concepts, any component should have a contract - even if there will be only one, ever.
This makes sense for public components but the case I'm describing is a component inside of a /internal/ directory so it's really a "contract with myself".
Question is more if you are willing to allow this code to be replaced by another implementation or not, more than if it makes sense or not at the time being, IMHO, because you don't want thousands of components everywhere obviously.
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
Although I'm tempted now to copy/paste your comment into the code, I'm not hesitant because this is not the XWiki Configuration infra, just one module's config.
If we hardcoded "xwiki.properties" all over the code, that would be well accepted as a disaster, why then is it ok to hardcode documentation about how XWiki's configuration works all over the codebase?
This class uses ConfigurationSource, whatever that might do.
/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?
If I want to know what's the return value I will only check the return description in javadoc, maybe not the sometimes length full description above of it ... As much as I love xwiki, I must say I'm not a bit fan of old core javadoc ;-) Take these ones for example: http://nexus.xwiki.org/nexus/service/local/repositories/releases/archive/org...) http://nexus.xwiki.org/nexus/service/local/repositories/releases/archive/org...) I dare any contributor to find what they do without looking at the code :)
As a result of the discussion with Sergiu, I have decided that public functions should have the same javadoc standards as usual and optional javadoc should only apply to private or /internal/ (inside of a directory called internal) classes and functions.
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?)
Business logic in the configuration? Should the configuration also be responsible for checking if the file exists?
/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
Skirting the question. Do you really think that a 3 line function called somethingOtherThanGetUser() needs a javadoc comment?
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.
I do want a reference to XWikiGuest, null has a different meaning in this context. There are of course many ways to solve a given problem but if you claim that the implementation detail of how the internals of this class behave are wrong and deprecated, the burden of proof is on you.
All this discussion is a proof that it was actually needed to comment this ! :) On my side if I just see "XWikiWebSocketScriptService#getUser", I have absolutely no idea of what it could return ... - current user ? - user of the web socket ? (if it means something) - user that owns a thread of the web socket service ? ... IMHO something called "getXxx" or "setXxx" is implicitely described only in case of a bean or pojo, in a script service or component interface there's nothing really obvious usually ...
This all makes perfect sense and I would agree entirely if it were not for the fact that it's a private method.
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.
That's a good point, I'll document those.
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.
I think what's really missing here is that they never really exposed an URL separator constant in the JDK ... But it's difficult for a checking-tool to differentiate between those url separators (that will never change, so who cares as you say), and using file paths separators hard-coded (which is pretty bad and create many environment specific weirdness in so many tools). I think if they did that with maven plugins from the start, they wouldn't be hunting for these '/' and '\' like they do, and they'd have lighten many JIRAs ...
I think the correct solution to windows insanity is to create a filesystem abstraction layer which deals with their their broken slashes and magical filenames which you can never use (eg: COM1) and their sort-of-but-not-quite international filenames based on UTF-16 which can *almost* represent all international characters. If that were implemented in an abstraction layer then it would save a whole lot of bug reports and a whole lot of pain and suffering trying to deal with it.
It's not more maintainable, it just looks more maintainable. If someone's hardcoding filesystem paths inside of classes then they're certainly doing so many other horrible things that the file paths should be preserved as a red flag for reviewers!
99% of strings are not file paths, they're error messages and forward slashes and other things which will never change. Even configuration keys are impossible to change once they're deployed in the field.
#define TEN 10
/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.
Because it's internal, I assume that you can just look at the implementation. But this gets back to the question of why we need to define interfaces for which there will never be more than one impl.
Default scope for javadocs with maven plugin is protected ... Not public. Personnally I (try) to put javadocs everywhere, even on private methods (Hey, I don't succeed, you can blame me ;) ) Because I describe there the contract of the method - not how it's implemented - there are non-javadoc comments for that. And often I'm the first who reads them, when I'm back to work after a long time : "so, what was this for again ... ?" Sometimes the name of the method is obvious, but almost all the time it's not sufficient. It's also a nice way to document a change - you see the change in contract, and impact on implementation, or you see that implementation changed, but not the contract. (when I say contract here it's more than the method signature)
But that's mainly my point of view because I like javadocs ! :)
/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:
Override indeed solves the checkstyle error.
/** * {@link WebSocket} implementation using the * <a href="http://netty.io/">Netty library</a>. */
Does telling the reader that Netty means netty.io make the code fundamentally better? If so then I could make all of my code better by telling the reader how to use google.
Personnally I don't like those kind of external links, because in some time from now they could easily have disappeared, leading to broken links everywhere in your javadoc ... I'd document these in the overall component/module/software documentation, in a wiki, in somewhere easily up-to-date.
/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.");
Easy fix is still worse than "not broken", I don't want to know how easy it is to trick this rule, I know a lot of tricks already. I want to hear a justification for this rule in the face of the fact that it makes development harder than it would otherwise be.
I think the fix should logically bring another broken rule about appending strings with '+' :D
I'm not sure where should be put the cursor, from "it makes development harder" compared to "it's an important rule", apart from applying rules that were agreed among a community ...
If I ever perform a modification on this code, let's say around the first RuntimeException, and I changed the name of the property to something else than "websocket.ssl.blabla", I could maybe easily miss the fact that this string appears somewhere else, then the user would get a different property name in different exceptions depending on the use-case, one good one wrong.
(blame me for being a bad developer not searching for this string in all the code if you want :), but remember, I'm a contributor, not a committer,
I won't, I believe form follows function, working code rules. But then that's what this thread is about :) However the example is not so great because configuration keys are impossible to change in practice.
you didn't hire me on my skills, you can just provide me as many recommendations as you want, and I may never follow them. Your only possibility is to review my code thoroughly (which will not improve time for merging contributed code), or rely on some tool to automate some basic checks.)
BTW IMO it should be the name of this property that should be in a constant, even before this exception message string.
For me this falls in the "I might do that when I refactor that code but it's not critical" category. I see your point.
/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.
I write code in a number of languages and I want to adopt habits which are as widely applicable as possible.
/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.
I've written functions much longer and much easier to read as well as much shorter functions which, due to the complexity of the algorithm, push my own mental capacity to it's limit.
Surely I still have much room for improvement in code craftsmanship but I don't think a checkstyle failure is any more likely to make me suddenly discover a clearer way to express a function any more than a rejected paper would suddenly make me a mathematician.
On the contrary, fixing these types of errors will almost certainly make the code *worse* because the original state of the function is an expression of the mental model of the author and in response to failures, he will break it up along arbitrary lines in order to make the errors stop, but the code and the programmer will not and cannot be fundamentally better than they were 20 minutes before.
I mostly agree with you on this. My personal idea is that there should be a limit, but far greater than the usual or expected limit. But maybe overriding the rule is enough on a case per case basis ... In my case I usually write much more statements than most developers would do, because I can't do another way :) I prefer to write all little steps, and let the compiler do its job, and it's the way I think it. I think my code would most of the time break this rule ... :/
/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, "?");
I don't want to see the tricks, I want to see compelling justification for the rule.
As any rule has exceptions, there are exceptions to the compelling justification for a rule ... Maybe some rules are bad, or badly specified, or there's just some rare cases where they're more annoying than useful, but would you disable them completely in all cases ?
My opinion is that we need to periodically review our rules, (as well as rules which we don't have and perhaps should!) We should ask a few questions: How much does a rule help when it's helpful? if the rule, for example, prevents deadlock conditions 1% of the time then maybe we're willing to accept more pain to keep using it. How much does it hurt when it misdiagnoses a non-problem as a problem? Is it hard to "fix"? Does "fixing" the code make it worse? How often is it accurate? How often is it inaccurate or overbearing? This is a tricky question because often a check triggers for one developer and not for another and the other developer is tempted to think that his associate is wrong. We need to be careful to differentiate "unacceptable" from "could be better" or "not the way I would have done it". Thanks, Caleb
(Why would any serious library use strings for representing URIs instead of java.net.URI?)
If I gave you a string and a handful of URI manipulating functions then even if you have no knowledge of those functions, you can still understand it as a plain old string, if I give you a java.net.URI then you have to go look up API documentation no matter what, an obvious cost increase.
If there are no solid justifications for this cost then I call overengineering.
/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.
It's not enforced by checkstyle, ftr this is one rule which I would like to see added (but only if we can at least balance the development pain budget, if not lower it).
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.
It will "probably never happen" but I fixed it for the next version, thanks for spotting that. Since it's used in an inner class, I though I needed it to be final but indeed using the field works, fixed.
So to summarize, most painful rules:
1. Multiple identical string constants in a file 2. Javadoc /internal/ classes and methods 3. (not a checkstyle rule) interfaces with one impl. 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.
Thanks, Caleb _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
_______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
-- Caleb James DeLisle XWiki SAS [email protected]
Hi, 2014-09-11 10:43 GMT+02:00 Caleb James DeLisle <[email protected]>:
Hi Jeremie,
Sorry for not replying sooner, I felt that I should give your mail a bit more thought and a little but more background about me since we don't know eachother as well as I know Sergiu and Marius.
No problem and I hesitated before answering, because I didn't want to feel harsh or misunderstood.
On 09/09/2014 06:02 PM, Jeremie BOUSQUET wrote:
Hi,
My own humble remarks ... :)
2014-09-09 11:00 GMT+02:00 Caleb James DeLisle <[email protected]>:
Firstly, thank you very much for reviewing my code.
When I was young I hated homework, my parents told me that if I spent as much time on the work as I spent bellyaching about it, I'd have it done already. A lot of your review seems to take the point of view that it's "not that hard", my counter is that no rule makes contributing *easier* and as contributors are not exactly lining up at the door, any rule that exists needs to justify itself as necessary.
There's the implicit idea here that a rule is something created to bother developers. Something that limit contribution, is costly eventually, requires an effort, and so on ... I think the concept and objectives of such rules is exactly the contrary, they're here to help contribution (by making the code clearer), and give a framework that everyone knows and understands (less effort to dig into code you didn't write).
Although in this mail thread I have been very critical, I should probably show my credentials as "pro best practice". I'm a leader of a project in C and I developed my own codestyle validator which checks, among other things, that all publicly accessible functions are prefixed with the name of the file where they're defined. Although I have on occasion wanted to violate this rule, I have yet to find what I think is a *valid* reason for doing so and I have so far kept it 100% which makes reading the code much easier for other people who upon seeing a function which they don't recognize, need only scroll to the top of the file to see which header it's defined in.
Thanks for the background, I didn't doubt about the fact that you care about the rules, because you wouldn't have sent such email if it weren't the case ;-) But it was the "feeling" I had by reading your text. On my side I've worked as developer for some time now, and along with this had for years a role of "rule enforcer", particularly regarding configuration management tools and processes, but also regarding the build... My approach has always been to explain things to justify chosen rules though, not to enforce a dictatorship (as is sometimes the case in private companies...). The discussions I read here about software engineering (to which I sometimes contribute modestly), are a breath of air for me I must say :)
Of course there are always rules that can be discussed, regarding their pertinence, criticality etc.
That's what I'm after, I feel that every rule imparts a certain amount of cost and we need to periodically review them to make sure that they are net-positive value.
That's very sane.
On 09/08/2014 07:30 PM, Sergiu Dumitriu wrote:
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"
This is the first time I've heard that. Indeed I don't like "Default" either but it seems we don't use "Impl". This question brings up a more difficult point, why do we need an interface for each thing we register in the CM, even if there's only ever going to be one impl?
The issue I see here, is answering the question : "only one implementation will ever make sense". Other contributors may invent over implementations you never thought about.
I've been here myself, I have made a lot of very nice interfaces to "future proof" my designs, and then when a new use case actually came along, I found my interfaces unduely difficult to use for that use case and I promptly re-factored the code.
Since then, I've developed a strong tendency toward YAGNI and "simplest thing that could possibly work". By the time someone finds a new use case, the code probably needs some refactoring anyway.
I quite agree with you, in this specific case my point is that extracting an interface is not a very high cost. It doesn't mean that you have to code complex things to generalize all your code based on those interfaces. It's just the level-0 of something that could potentially be re-used - and if it's never re-used, which is probable, then it didn't cost much. Personnally, as I'm not very smart in some ways, and my memory is frequently faulty, those interfaces are useful first to me, so I remember what was the contract I was trying to implement ;) I didn't apply simplicity (or didn't succeed to) for some parts of the mail archive I code, that I've rewritten recently, and I now find those parts totally horrible :D "do what I say not what I do ..." But I think it's a discussion that's a bit far from a pure coding rules checking, it's more an architecture thing.
Following IOC and java interfaces concepts, any component should have a contract - even if there will be only one, ever.
This makes sense for public components but the case I'm describing is a component inside of a /internal/ directory so it's really a "contract with myself".
In my mind, it was not just a discussion for coding rules of "public" things. It's not just for people that would like to use those methods (not needed they're private), but also for people wanting to contribute to this code. If it's deadly obvious from the code what it does, then I agree with you, javadoc, comments and interfaces are less needed, but it's very often very obvious only to the person who wrote the code. Of course it may be a different approach depending on supposed skills of development teams. I won't talk about my work here :D
Question is more if you are willing to allow this code to be replaced by another implementation or not, more than if it makes sense or not at the time being, IMHO, because you don't want thousands of components everywhere obviously.
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
Although I'm tempted now to copy/paste your comment into the code, I'm not hesitant because this is not the XWiki Configuration infra, just one module's config.
If we hardcoded "xwiki.properties" all over the code, that would be well accepted as a disaster, why then is it ok to hardcode documentation about how XWiki's configuration works all over the codebase?
This class uses ConfigurationSource, whatever that might do.
/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?
If I want to know what's the return value I will only check the return description in javadoc, maybe not the sometimes length full description above of it ... As much as I love xwiki, I must say I'm not a bit fan of old core javadoc ;-) Take these ones for example:
http://nexus.xwiki.org/nexus/service/local/repositories/releases/archive/org...)
http://nexus.xwiki.org/nexus/service/local/repositories/releases/archive/org...)
I dare any contributor to find what they do without looking at the code :)
As a result of the discussion with Sergiu, I have decided that public functions should have the same javadoc standards as usual and optional javadoc should only apply to private or /internal/ (inside of a directory called internal) classes and functions.
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?)
Business logic in the configuration? Should the configuration also be responsible for checking if the file exists?
/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
Skirting the question. Do you really think that a 3 line function called somethingOtherThanGetUser() needs a javadoc comment?
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.
I do want a reference to XWikiGuest, null has a different meaning in this context. There are of course many ways to solve a given problem but if you claim that the implementation detail of how the internals of this class behave are wrong and deprecated, the burden of proof is on you.
All this discussion is a proof that it was actually needed to comment this ! :) On my side if I just see "XWikiWebSocketScriptService#getUser", I have absolutely no idea of what it could return ... - current user ? - user of the web socket ? (if it means something) - user that owns a thread of the web socket service ? ... IMHO something called "getXxx" or "setXxx" is implicitely described only in case of a bean or pojo, in a script service or component interface there's nothing really obvious usually ...
This all makes perfect sense and I would agree entirely if it were not for the fact that it's a private method.
But what's the real difference ? private is a java keyword. Private methods are as "open-sourced" as the others ;) If I ever want to contribute a fix of a bug in this code, one day, it may be helpful to me, to know what you intended to do when you wrote this method. Because if a bug is found, it would mean that what you implemented was not quite like what you intended to. But I admit I may be a bit radical and theoretical here.
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.
That's a good point, I'll document those.
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.
I think what's really missing here is that they never really exposed an URL separator constant in the JDK ... But it's difficult for a checking-tool to differentiate between those url separators (that will never change, so who cares as you say), and using file paths separators hard-coded (which is pretty bad and create many environment specific weirdness in so many tools). I think if they did that with maven plugins from the start, they wouldn't be hunting for these '/' and '\' like they do, and they'd have lighten many JIRAs ...
I think the correct solution to windows insanity is to create a filesystem abstraction layer which deals with their their broken slashes and magical filenames which you can never use (eg: COM1) and their sort-of-but-not-quite international filenames based on UTF-16 which can *almost* represent all international characters.
If that were implemented in an abstraction layer then it would save a whole lot of bug reports and a whole lot of pain and suffering trying to deal with it.
It's not more maintainable, it just looks more maintainable. If someone's hardcoding filesystem paths inside of classes then they're certainly doing so many other horrible things that the file paths should be preserved as a red flag for reviewers!
99% of strings are not file paths, they're error messages and forward slashes and other things which will never change. Even configuration keys are impossible to change once they're deployed in the field.
#define TEN 10
/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.
Because it's internal, I assume that you can just look at the implementation. But this gets back to the question of why we need to define interfaces for which there will never be more than one impl.
Default scope for javadocs with maven plugin is protected ... Not public. Personnally I (try) to put javadocs everywhere, even on private methods (Hey, I don't succeed, you can blame me ;) ) Because I describe there the contract of the method - not how it's implemented - there are non-javadoc comments for that. And often I'm the first who reads them, when I'm back to work after a long time : "so, what was this for again ... ?" Sometimes the name of the method is obvious, but almost all the time it's not sufficient. It's also a nice way to document a change - you see the change in contract, and impact on implementation, or you see that implementation changed, but not the contract. (when I say contract here it's more than the method signature)
But that's mainly my point of view because I like javadocs ! :)
/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:
Override indeed solves the checkstyle error.
/** * {@link WebSocket} implementation using the * <a href="http://netty.io/">Netty library</a>. */
Does telling the reader that Netty means netty.io make the code fundamentally better? If so then I could make all of my code better by telling the reader how to use google.
Personnally I don't like those kind of external links, because in some time from now they could easily have disappeared, leading to broken links everywhere in your javadoc ... I'd document these in the overall component/module/software documentation, in a wiki, in somewhere easily up-to-date.
/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.");
Easy fix is still worse than "not broken", I don't want to know how easy it is to trick this rule, I know a lot of tricks already. I want to hear a justification for this rule in the face of the fact that it makes development harder than it would otherwise be.
I think the fix should logically bring another broken rule about appending strings with '+' :D
I'm not sure where should be put the cursor, from "it makes development harder" compared to "it's an important rule", apart from applying rules that were agreed among a community ...
If I ever perform a modification on this code, let's say around the first RuntimeException, and I changed the name of the property to something else than "websocket.ssl.blabla", I could maybe easily miss the fact that this string appears somewhere else, then the user would get a different property name in different exceptions depending on the use-case, one good one wrong.
(blame me for being a bad developer not searching for this string in all the code if you want :), but remember, I'm a contributor, not a committer,
I won't, I believe form follows function, working code rules. But then that's what this thread is about :) However the example is not so great because configuration keys are impossible to change in practice.
you didn't hire me on my skills, you can just provide me as many recommendations as you want, and I may never follow them. Your only possibility is to review my code thoroughly (which will not improve time for merging contributed code), or rely on some tool to automate some basic checks.)
BTW IMO it should be the name of this property that should be in a constant, even before this exception message string.
For me this falls in the "I might do that when I refactor that code but it's not critical" category. I see your point.
/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.
I write code in a number of languages and I want to adopt habits which are as widely applicable as possible.
/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.
I've written functions much longer and much easier to read as well as much shorter functions which, due to the complexity of the algorithm, push my own mental capacity to it's limit.
Surely I still have much room for improvement in code craftsmanship but I don't think a checkstyle failure is any more likely to make me suddenly discover a clearer way to express a function any more than a rejected paper would suddenly make me a mathematician.
On the contrary, fixing these types of errors will almost certainly make the code *worse* because the original state of the function is an expression of the mental model of the author and in response to failures, he will break it up along arbitrary lines in order to make the errors stop, but the code and the programmer will not and cannot be fundamentally better than they were 20 minutes before.
I mostly agree with you on this. My personal idea is that there should be a limit, but far greater than the usual or expected limit. But maybe overriding the rule is enough on a case per case basis ... In my case I usually write much more statements than most developers would do, because I can't do another way :) I prefer to write all little steps, and let the compiler do its job, and it's the way I think it. I think my code would most of the time break this rule ... :/
/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, "?");
I don't want to see the tricks, I want to see compelling justification for the rule.
As any rule has exceptions, there are exceptions to the compelling justification for a rule ... Maybe some rules are bad, or badly specified, or there's just some rare cases where they're more annoying than useful, but would you disable them completely in all cases ?
My opinion is that we need to periodically review our rules, (as well as rules which we don't have and perhaps should!)
We should ask a few questions:
How much does a rule help when it's helpful? if the rule, for example, prevents deadlock conditions 1% of the time then maybe we're willing to accept more pain to keep using it.
How much does it hurt when it misdiagnoses a non-problem as a problem? Is it hard to "fix"? Does "fixing" the code make it worse?
How often is it accurate?
Those are very difficult questions to answer, depending on the rules. It's also difficult to define what is a code becoming "worse", because of what you say just below about accuracy and "could be better" and so on.
How often is it inaccurate or overbearing? This is a tricky question because often a check triggers for one developer and not for another and the other developer is tempted to think that his associate is wrong. We need to be careful to differentiate "unacceptable" from "could be better" or "not the way I would have done it".
That's a choice of what criticality to what problem, but the "not the way I would have done it", normally already has been discussed when the rules were decided. It's like the law then, either you follow it, either you change it ;) But I agree it's tricky. By the way I understood this thread more as think-tank, but if you intended to formalize some decisions regarding rules I apologize and will stop, because I'm more scattering attention than focusing on solutions I'm afraid ...
Thanks, Caleb
(Why would any serious library use strings for representing URIs
instead
of java.net.URI?)
If I gave you a string and a handful of URI manipulating functions then even if you have no knowledge of those functions, you can still understand it as a plain old string, if I give you a java.net.URI then you have to go look up API documentation no matter what, an obvious cost increase.
If there are no solid justifications for this cost then I call overengineering.
/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.
It's not enforced by checkstyle, ftr this is one rule which I would like to see added (but only if we can at least balance the development pain budget, if not lower it).
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.
It will "probably never happen" but I fixed it for the next version, thanks for spotting that. Since it's used in an inner class, I though I needed it to be final but indeed using the field works, fixed.
So to summarize, most painful rules:
1. Multiple identical string constants in a file 2. Javadoc /internal/ classes and methods 3. (not a checkstyle rule) interfaces with one impl. 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.
Thanks, Caleb _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
_______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
-- Caleb James DeLisle XWiki SAS [email protected] _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
On Tue, Sep 9, 2014 at 12:00 PM, Caleb James DeLisle <[email protected]> wrote:
Firstly, thank you very much for reviewing my code.
When I was young I hated homework, my parents told me that if I spent as much time on the work as I spent bellyaching about it, I'd have it done already. A lot of your review seems to take the point of view that it's "not that hard", my counter is that no rule makes contributing *easier* and as contributors are not exactly lining up at the door, any rule that exists needs to justify itself as necessary.
On 09/08/2014 07:30 PM, Sergiu Dumitriu wrote:
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"
This is the first time I've heard that. Indeed I don't like "Default" either but it seems we don't use "Impl". This question brings up a more difficult point, why do we need an interface for each thing we register in the CM, even if there's only ever going to be one impl?
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
Although I'm tempted now to copy/paste your comment into the code, I'm not hesitant because this is not the XWiki Configuration infra, just one module's config.
If we hardcoded "xwiki.properties" all over the code, that would be well accepted as a disaster, why then is it ok to hardcode documentation about how XWiki's configuration works all over the codebase?
This class uses ConfigurationSource, whatever that might do.
/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?)
Business logic in the configuration? Should the configuration also be responsible for checking if the file exists?
/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
Skirting the question.
Do you really think that a 3 line function called somethingOtherThanGetUser() needs a javadoc comment?
You just said "Statements per function is almost useless as a measure of code complexity". I suppose you mean it both ways. * Less statements (and lines) doesn't imply less complexity * More statements (and lines) doesn't imply more complexity So the requirement of javadoc should not be decided based on the number of statements (lines).
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.
I do want a reference to XWikiGuest, null has a different meaning in this context. There are of course many ways to solve a given problem but if you claim that the implementation detail of how the internals of this class behave are wrong and deprecated, the burden of proof is on you.
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.
That's a good point, I'll document those.
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.
A workaround is to use the '/' character (instead of string) but then you have the "magic number" problem. Anyway, I still consider this a good rule for the arguments mentioned by Sergiu.
It's not more maintainable, it just looks more maintainable. If someone's hardcoding filesystem paths inside of classes then they're certainly doing so many other horrible things that the file paths should be preserved as a red flag for reviewers!
99% of strings are not file paths, they're error messages and forward slashes and other things which will never change. Even configuration keys are impossible to change once they're deployed in the field.
#define TEN 10
/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.
Because it's internal, I assume that you can just look at the implementation. But this gets back to the question of why we need to define interfaces for which there will never be more than one impl.
/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:
Override indeed solves the checkstyle error.
/** * {@link WebSocket} implementation using the * <a href="http://netty.io/">Netty library</a>. */
Does telling the reader that Netty means netty.io make the code fundamentally better? If so then I could make all of my code better by telling the reader how to use google.
/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.");
Easy fix is still worse than "not broken", I don't want to know how easy it is to trick this rule, I know a lot of tricks already. I want to hear a justification for this rule in the face of the fact that it makes development harder than it would otherwise be.
/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.
The reason for me is this: You write a list like this * item one * item two not like this * item one * item two '*' is an operator here (AND/OR). It's best to put the operator at the start of the line when the expression is split on multiple lines because we scan the text from left to right and it's quicker to spot the operator at the start (which is also vertically aligned) than at the end (which is not aligned).
I write code in a number of languages and I want to adopt habits which are as widely applicable as possible.
/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.
I've written functions much longer and much easier to read as well as much shorter functions which, due to the complexity of the algorithm, push my own mental capacity to it's limit.
Surely I still have much room for improvement in code craftsmanship but I don't think a checkstyle failure is any more likely to make me suddenly discover a clearer way to express a function any more than a rejected paper would suddenly make me a mathematician.
The goal of the checkstyle failure is to warn you that something might not be right. "Hey, are you sure this method is not too long? Can it be split?". And you can say "No!". It's up to you to decide. We cannot have rules that are right in 100% or the cases. But I think it's good that when the method exceeds a number of statements (be it 30 or something else) you stop for a second and ask yourself if it can be split. This checkstyle rule reminds us to do this, so I think it's useful.
On the contrary, fixing these types of errors will almost certainly make the code *worse* because the original state of the function is an expression of the mental model of the author and in response to failures, he will break it up along arbitrary lines in order to make the errors stop, but the code and the programmer will not and cannot be fundamentally better than they were 20 minutes before.
/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, "?");
I don't want to see the tricks, I want to see compelling justification for the rule.
(Why would any serious library use strings for representing URIs instead of java.net.URI?)
If I gave you a string and a handful of URI manipulating functions then even if you have no knowledge of those functions, you can still understand it as a plain old string, if I give you a java.net.URI then you have to go look up API documentation no matter what, an obvious cost increase.
If there are no solid justifications for this cost then I call overengineering.
/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.
It's not enforced by checkstyle, ftr this is one rule which I would like to see added (but only if we can at least balance the development pain budget, if not lower it).
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.
It will "probably never happen" but I fixed it for the next version, thanks for spotting that. Since it's used in an inner class, I though I needed it to be final but indeed using the field works, fixed.
So to summarize, most painful rules:
1. Multiple identical string constants in a file 2. Javadoc /internal/ classes and methods 3. (not a checkstyle rule) interfaces with one impl. 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.
Thanks, Caleb _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
On 09/11/2014 08:38 AM, Marius Dumitru Florea wrote:
On Tue, Sep 9, 2014 at 12:00 PM, Caleb James DeLisle <[email protected]> wrote:
Firstly, thank you very much for reviewing my code.
When I was young I hated homework, my parents told me that if I spent as much time on the work as I spent bellyaching about it, I'd have it done already. A lot of your review seems to take the point of view that it's "not that hard", my counter is that no rule makes contributing *easier* and as contributors are not exactly lining up at the door, any rule that exists needs to justify itself as necessary.
On 09/08/2014 07:30 PM, Sergiu Dumitriu wrote:
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"
This is the first time I've heard that. Indeed I don't like "Default" either but it seems we don't use "Impl". This question brings up a more difficult point, why do we need an interface for each thing we register in the CM, even if there's only ever going to be one impl?
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
Although I'm tempted now to copy/paste your comment into the code, I'm not hesitant because this is not the XWiki Configuration infra, just one module's config.
If we hardcoded "xwiki.properties" all over the code, that would be well accepted as a disaster, why then is it ok to hardcode documentation about how XWiki's configuration works all over the codebase?
This class uses ConfigurationSource, whatever that might do.
/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?)
Business logic in the configuration? Should the configuration also be responsible for checking if the file exists?
/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
Skirting the question.
Do you really think that a 3 line function called somethingOtherThanGetUser() needs a javadoc comment?
You just said "Statements per function is almost useless as a measure of code complexity". I suppose you mean it both ways.
* Less statements (and lines) doesn't imply less complexity * More statements (and lines) doesn't imply more complexity
So the requirement of javadoc should not be decided based on the number of statements (lines).
I think the decision of what type of javadoc (if any) should be up to the developer in the case of /internal/ or private functions. In the case of public/external functions, I'm happy with the current javadoc standard.
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.
I do want a reference to XWikiGuest, null has a different meaning in this context. There are of course many ways to solve a given problem but if you claim that the implementation detail of how the internals of this class behave are wrong and deprecated, the burden of proof is on you.
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.
That's a good point, I'll document those.
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.
A workaround is to use the '/' character (instead of string) but then you have the "magic number" problem.
Anyway, I still consider this a good rule for the arguments mentioned by Sergiu.
Can you elaborate on which arguments you thought were important because AFAICT, the last thing he had to say on the topic was: `` 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. ''
It's not more maintainable, it just looks more maintainable. If someone's hardcoding filesystem paths inside of classes then they're certainly doing so many other horrible things that the file paths should be preserved as a red flag for reviewers!
99% of strings are not file paths, they're error messages and forward slashes and other things which will never change. Even configuration keys are impossible to change once they're deployed in the field.
#define TEN 10
/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.
Because it's internal, I assume that you can just look at the implementation. But this gets back to the question of why we need to define interfaces for which there will never be more than one impl.
/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:
Override indeed solves the checkstyle error.
/** * {@link WebSocket} implementation using the * <a href="http://netty.io/">Netty library</a>. */
Does telling the reader that Netty means netty.io make the code fundamentally better? If so then I could make all of my code better by telling the reader how to use google.
/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.");
Easy fix is still worse than "not broken", I don't want to know how easy it is to trick this rule, I know a lot of tricks already. I want to hear a justification for this rule in the face of the fact that it makes development harder than it would otherwise be.
/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.
The reason for me is this:
You write a list like this
* item one * item two
not like this
* item one * item two
'*' is an operator here (AND/OR). It's best to put the operator at the start of the line when the expression is split on multiple lines because we scan the text from left to right and it's quicker to spot the operator at the start (which is also vertically aligned) than at the end (which is not aligned).
I don't think wiki syntax is can be reasonably compared to programming but I do think it's reasonable to want to use the same idiums in many C-derived languages. Your argument about alignment sounds reasonable but a counter-argument is that if the + is at the beginning of the line, the line must be misalligned in order to keep the tabbing the same. callFunction("a very long string " + "which is not really aligned"); vs. callFunction("another long string " + "doesn't this look better?"); But this is a sort of rule that I don't care too much about and I'm ok with leaving it as it is. It is however disturbing to me that we frequently have "bikeshed painting parties" over such matters of triviality when there are clearly more serious issues to be dealt with.
I write code in a number of languages and I want to adopt habits which are as widely applicable as possible.
/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.
I've written functions much longer and much easier to read as well as much shorter functions which, due to the complexity of the algorithm, push my own mental capacity to it's limit.
Surely I still have much room for improvement in code craftsmanship but I don't think a checkstyle failure is any more likely to make me suddenly discover a clearer way to express a function any more than a rejected paper would suddenly make me a mathematician.
The goal of the checkstyle failure is to warn you that something might not be right. "Hey, are you sure this method is not too long? Can it be split?". And you can say "No!". It's up to you to decide. We cannot have rules that are right in 100% or the cases. But I think it's good
Some rules will more or less always be right, trailing whitespace comes to mind here, I have this rule in the cjdns build and I am in the habit of making trailing whitespace when I am working on something and not complete so the rule serves also as a way to mark my location when I need to restart my work later.
that when the method exceeds a number of statements (be it 30 or something else) you stop for a second and ask yourself if it can be split. This checkstyle rule reminds us to do this, so I think it's useful.
This could be argued as useful if it didn't fail the build. As it is we either have to split the function (almost certainly making it worse) or add a checkstyle exception and forgo all of the verifications which checkstyle has to offer on the entire file. Thanks, Caleb
On the contrary, fixing these types of errors will almost certainly make the code *worse* because the original state of the function is an expression of the mental model of the author and in response to failures, he will break it up along arbitrary lines in order to make the errors stop, but the code and the programmer will not and cannot be fundamentally better than they were 20 minutes before.
/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, "?");
I don't want to see the tricks, I want to see compelling justification for the rule.
(Why would any serious library use strings for representing URIs instead of java.net.URI?)
If I gave you a string and a handful of URI manipulating functions then even if you have no knowledge of those functions, you can still understand it as a plain old string, if I give you a java.net.URI then you have to go look up API documentation no matter what, an obvious cost increase.
If there are no solid justifications for this cost then I call overengineering.
/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.
It's not enforced by checkstyle, ftr this is one rule which I would like to see added (but only if we can at least balance the development pain budget, if not lower it).
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.
It will "probably never happen" but I fixed it for the next version, thanks for spotting that. Since it's used in an inner class, I though I needed it to be final but indeed using the field works, fixed.
So to summarize, most painful rules:
1. Multiple identical string constants in a file 2. Javadoc /internal/ classes and methods 3. (not a checkstyle rule) interfaces with one impl. 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.
Thanks, Caleb _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
_______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
On Thu, Sep 11, 2014 at 9:10 AM, Caleb James DeLisle <[email protected]> wrote:
On 09/11/2014 08:38 AM, Marius Dumitru Florea wrote:
On Tue, Sep 9, 2014 at 12:00 PM, Caleb James DeLisle <[email protected]> wrote:
Firstly, thank you very much for reviewing my code.
When I was young I hated homework, my parents told me that if I spent as much time on the work as I spent bellyaching about it, I'd have it done already. A lot of your review seems to take the point of view that it's "not that hard", my counter is that no rule makes contributing *easier* and as contributors are not exactly lining up at the door, any rule that exists needs to justify itself as necessary.
On 09/08/2014 07:30 PM, Sergiu Dumitriu wrote:
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"
This is the first time I've heard that. Indeed I don't like "Default" either but it seems we don't use "Impl". This question brings up a more difficult point, why do we need an interface for each thing we register in the CM, even if there's only ever going to be one impl?
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
Although I'm tempted now to copy/paste your comment into the code, I'm not hesitant because this is not the XWiki Configuration infra, just one module's config.
If we hardcoded "xwiki.properties" all over the code, that would be well accepted as a disaster, why then is it ok to hardcode documentation about how XWiki's configuration works all over the codebase?
This class uses ConfigurationSource, whatever that might do.
/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?)
Business logic in the configuration? Should the configuration also be responsible for checking if the file exists?
/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
Skirting the question.
Do you really think that a 3 line function called somethingOtherThanGetUser() needs a javadoc comment?
You just said "Statements per function is almost useless as a measure of code complexity". I suppose you mean it both ways.
* Less statements (and lines) doesn't imply less complexity * More statements (and lines) doesn't imply more complexity
So the requirement of javadoc should not be decided based on the number of statements (lines).
I think the decision of what type of javadoc (if any) should be up to the developer in the case of /internal/ or private functions. In the case of public/external functions, I'm happy with the current javadoc standard.
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.
I do want a reference to XWikiGuest, null has a different meaning in this context. There are of course many ways to solve a given problem but if you claim that the implementation detail of how the internals of this class behave are wrong and deprecated, the burden of proof is on you.
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.
That's a good point, I'll document those.
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.
A workaround is to use the '/' character (instead of string) but then you have the "magic number" problem.
Anyway, I still consider this a good rule for the arguments mentioned by Sergiu.
Can you elaborate on which arguments you thought were important because AFAICT, the last thing he had to say on the topic was:
`` 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. ''
It's not more maintainable, it just looks more maintainable. If someone's hardcoding filesystem paths inside of classes then they're certainly doing so many other horrible things that the file paths should be preserved as a red flag for reviewers!
99% of strings are not file paths, they're error messages and forward slashes and other things which will never change. Even configuration keys are impossible to change once they're deployed in the field.
#define TEN 10
/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.
Because it's internal, I assume that you can just look at the implementation. But this gets back to the question of why we need to define interfaces for which there will never be more than one impl.
/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:
Override indeed solves the checkstyle error.
/** * {@link WebSocket} implementation using the * <a href="http://netty.io/">Netty library</a>. */
Does telling the reader that Netty means netty.io make the code fundamentally better? If so then I could make all of my code better by telling the reader how to use google.
/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.");
Easy fix is still worse than "not broken", I don't want to know how easy it is to trick this rule, I know a lot of tricks already. I want to hear a justification for this rule in the face of the fact that it makes development harder than it would otherwise be.
/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.
The reason for me is this:
You write a list like this
* item one * item two
not like this
* item one * item two
'*' is an operator here (AND/OR). It's best to put the operator at the start of the line when the expression is split on multiple lines because we scan the text from left to right and it's quicker to spot the operator at the start (which is also vertically aligned) than at the end (which is not aligned).
I don't think wiki syntax is can be reasonably compared to programming but I do think it's reasonable to want to use the same idiums in many C-derived languages.
Your argument about alignment sounds reasonable but a counter-argument is that if the + is at the beginning of the line, the line must be misalligned in order to keep the tabbing the same.
callFunction("a very long string " + "which is not really aligned");
vs.
callFunction("another long string " + "doesn't this look better?");
But this is a sort of rule that I don't care too much about and I'm ok with leaving it as it is. It is however disturbing to me that we frequently have "bikeshed painting parties" over such matters of triviality when there are clearly more serious issues to be dealt with.
I write code in a number of languages and I want to adopt habits which are as widely applicable as possible.
/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.
I've written functions much longer and much easier to read as well as much shorter functions which, due to the complexity of the algorithm, push my own mental capacity to it's limit.
Surely I still have much room for improvement in code craftsmanship but I don't think a checkstyle failure is any more likely to make me suddenly discover a clearer way to express a function any more than a rejected paper would suddenly make me a mathematician.
The goal of the checkstyle failure is to warn you that something might not be right. "Hey, are you sure this method is not too long? Can it be split?". And you can say "No!". It's up to you to decide. We cannot have rules that are right in 100% or the cases. But I think it's good
Some rules will more or less always be right, trailing whitespace comes to mind here, I have this rule in the cjdns build and I am in the habit of making trailing whitespace when I am working on something and not complete so the rule serves also as a way to mark my location when I need to restart my work later.
that when the method exceeds a number of statements (be it 30 or something else) you stop for a second and ask yourself if it can be split. This checkstyle rule reminds us to do this, so I think it's useful.
This could be argued as useful if it didn't fail the build. As it is we either have to split the function (almost certainly making it worse) or add a checkstyle exception and forgo all of the verifications which checkstyle has to offer on the entire file.
Not exactly, it is possible to disabled just one specific failure in the file, just not trough the pom.xml like we usually. See http://maven.apache.org/plugins/maven-checkstyle-plugin/examples/suppression....
Thanks, Caleb
On the contrary, fixing these types of errors will almost certainly make the code *worse* because the original state of the function is an expression of the mental model of the author and in response to failures, he will break it up along arbitrary lines in order to make the errors stop, but the code and the programmer will not and cannot be fundamentally better than they were 20 minutes before.
/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, "?");
I don't want to see the tricks, I want to see compelling justification for the rule.
(Why would any serious library use strings for representing URIs instead of java.net.URI?)
If I gave you a string and a handful of URI manipulating functions then even if you have no knowledge of those functions, you can still understand it as a plain old string, if I give you a java.net.URI then you have to go look up API documentation no matter what, an obvious cost increase.
If there are no solid justifications for this cost then I call overengineering.
/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.
It's not enforced by checkstyle, ftr this is one rule which I would like to see added (but only if we can at least balance the development pain budget, if not lower it).
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.
It will "probably never happen" but I fixed it for the next version, thanks for spotting that. Since it's used in an inner class, I though I needed it to be final but indeed using the field works, fixed.
So to summarize, most painful rules:
1. Multiple identical string constants in a file 2. Javadoc /internal/ classes and methods 3. (not a checkstyle rule) interfaces with one impl. 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.
Thanks, Caleb _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
_______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
_______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
-- Thomas Mortagne
participants (6)
-
Caleb James DeLisle -
Jeremie BOUSQUET -
Marius Dumitru Florea -
Sergiu Dumitriu -
Sergiu Dumitriu -
Thomas Mortagne