8 Sep
2014
8 Sep
'14
11:14 a.m.
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
8 Sep
8 Sep
7:30 p.m.
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/ma…
now references "nws" which became this.nws instead of nwsLocal.
--
Sergiu Dumitriu
http://purl.org/net/sergiu
9 Sep
9 Sep
11 a.m.
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:
Business logic in the configuration?
Should the configuration also be responsible for checking if the file exists?
Skirting the question.
Do you really think that a 3 line function called somethingOtherThanGetUser() needs a
javadoc comment?
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
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.
Override indeed solves the checkstyle error.
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 write code in a number of languages and I want to adopt habits which
are as widely applicable as possible.
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 don't want to see the tricks, I want to see compelling justification for
the rule.
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).
I'm gonna audit your audit audit.
On 09/08/2014 05:14 AM, Caleb James DeLisle wrote:
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?
/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
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?)
/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.
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.
/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>.
*/
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.");
/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?)
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.
Having both rules is a bit redundant, since the this rule is supposed to
allow the field hiding that the second rule is complaining about.
Still, why do you need to declare nws instead of assigning to this.nws?
As I can see, there's no exception that could stop the method between
the nwsLocal declaration and the this.nws assignment.
And I think you just introduced a bug,
https://github.com/xwiki-contrib/xwiki-contrib-websocket/blob/master/src/ma…
now references "nws" which became this.nws instead of nwsLocal.
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
3:22 p.m.
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
6:05 p.m.
On 09/09/2014 03:22 PM, Sergiu Dumitriu wrote:
Being able to write free-form inside of a module makes it IMO worth it.
Will do.
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
On 09/09/14 05:00, Caleb James DeLisle wrote:
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.
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.
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. 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 :)
6:02 p.m.
Hi,
My own humble remarks ... :)
2014-09-09 11:00 GMT+02:00 Caleb James DeLisle <cjd(a)cjdns.fr>fr>:
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/or…
http://nexus.xwiki.org/nexus/service/local/repositories/releases/archive/or…
I dare any contributor to find what they do without looking at the code :)
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 ...
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 ...
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.
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.
I write code in a number of languages and I want to adopt habits which
are as widely applicable as possible.
>
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 ... :/
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 ?
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).
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:
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.
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
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
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/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. >
/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?)
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.
Having both rules is a bit redundant, since the this rule is supposed to
allow the field hiding that the second rule is complaining about.
Still, why do you need to declare nws instead of assigning to this.nws?
As I can see, there's no exception that could stop the method between
the nwsLocal declaration and the this.nws assignment.
And I think you just introduced a bug,
https://github.com/xwiki-contrib/xwiki-contrib-websocket/blob/master/src/ma…
now references "nws" which became
this.nws instead of nwsLocal.
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
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
11 Sep
11 Sep
10:43 a.m.
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:
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.
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.
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.
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.
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.
I write code in a number of languages and I want to adopt habits which
are as widely applicable as possible.
>
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 ... :/
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
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).
_______________________________________________
devs mailing list
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
--
Caleb James DeLisle
XWiki SAS
calebjamesdelisle(a)xwiki.com
Hi,
My own humble remarks ... :)
2014-09-09 11:00 GMT+02:00 Caleb James DeLisle <cjd(a)cjdns.fr>fr>:
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.
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.
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:
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'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"
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.
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/or…
http://nexus.xwiki.org/nexus/service/local/repositories/releases/archive/or…
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.
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
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/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.
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. >
/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?)
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.
Having both rules is a bit redundant, since the this rule is supposed to
allow the field hiding that the second rule is complaining about.
Still, why do you need to declare nws instead of assigning to this.nws?
As I can see, there's no exception that could stop the method between
the nwsLocal declaration and the this.nws assignment.
And I think you just introduced a bug,
https://github.com/xwiki-contrib/xwiki-contrib-websocket/blob/master/src/ma…
now references "nws" which became
this.nws instead of nwsLocal.
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
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
11:48 a.m.
Hi,
2014-09-11 10:43 GMT+02:00 Caleb James DeLisle <cjd(a)cjdns.fr>fr>:
That's very sane.
implementation
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.
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
http://nexus.xwiki.org/nexus/service/local/repositories/releases/archive/or…
http://nexus.xwiki.org/nexus/service/local/repositories/releases/archive/or…
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
> All this discussion is a proof that
it was actually needed to comment
this
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.
URL
long
time
I think the fix should logically bring another broken rule about appending
be a
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.
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:
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 :)
Hi,
My own humble remarks ... :)
2014-09-09 11:00 GMT+02:00 Caleb James DeLisle <cjd(a)cjdns.fr>fr>:
a
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 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 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.
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:
> Do you think my freeform documentation is
less useful to a reader than
it would be if it contained
> a @return tag?
I dare any contributor to find what they do
without looking at the code
:)
>> 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.
>>
! :)
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
I think what's really missing here is that they never really exposed an > 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.
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 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 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.
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 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 ?
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
--
Caleb James DeLisle
XWiki SAS
calebjamesdelisle(a)xwiki.com
_______________________________________________
devs mailing list
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
>
>
>>
>> (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/ma…
>> 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
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
_______________________________________________
devs mailing list
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
8:38 a.m.
On Tue, Sep 9, 2014 at 12:00 PM, Caleb James DeLisle <cjd(a)cjdns.fr> wrote:
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.
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:
Business logic in the configuration?
Should the configuration also be responsible for checking if the file exists?
Skirting the question.
I'm gonna audit your audit audit.
On 09/08/2014 05:14 AM, Caleb James DeLisle wrote:
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?
/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
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?)
/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 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.
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
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.
Override indeed solves the checkstyle error.
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.
>
/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>.
*/
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.");
> 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.
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.
/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.
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 don't want to see the tricks, I want to see compelling justification for
the rule.
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).
/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?)
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.
Having both rules is a bit redundant, since the this rule is supposed to
allow the field hiding that the second rule is complaining about.
Still, why do you need to declare nws instead of assigning to this.nws?
As I can see, there's no exception that could stop the method between
the nwsLocal declaration and the this.nws assignment.
And I think you just introduced a bug,
https://github.com/xwiki-contrib/xwiki-contrib-websocket/blob/master/src/ma…
now references "nws" which became this.nws instead of nwsLocal.
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
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
9:10 a.m.
On 09/11/2014 08:38 AM, Marius Dumitru Florea wrote:
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.
''
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.
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.
On Tue, Sep 9, 2014 at 12:00 PM, Caleb James DeLisle
<cjd(a)cjdns.fr> wrote:
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.
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:
Business logic in the configuration?
Should the configuration also be responsible for checking if the file exists?
Skirting the question.
I'm gonna audit your audit audit.
On 09/08/2014 05:14 AM, Caleb James DeLisle wrote:
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?
/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
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?)
/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 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.
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
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.
Override indeed solves the checkstyle error.
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.
>
/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>.
*/
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.");
> 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.
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
/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. 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.
I don't want to see the tricks, I want to see compelling justification for
the rule.
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).
_______________________________________________
devs mailing list
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
/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?)
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.
Having both rules is a bit redundant, since the this rule is supposed to
allow the field hiding that the second rule is complaining about.
Still, why do you need to declare nws instead of assigning to this.nws?
As I can see, there's no exception that could stop the method between
the nwsLocal declaration and the this.nws assignment.
And I think you just introduced a bug,
https://github.com/xwiki-contrib/xwiki-contrib-websocket/blob/master/src/ma…
now references "nws" which became this.nws instead of nwsLocal.
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
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs 
9:27 a.m.
On Thu, Sep 11, 2014 at 9:10 AM, Caleb James DeLisle <cjd(a)cjdns.fr> wrote:
On 09/11/2014 08:38 AM, Marius Dumitru Florea wrote:
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.
''
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.
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.
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/suppressio….
On Tue, Sep 9, 2014 at 12:00 PM, Caleb James
DeLisle <cjd(a)cjdns.fr> wrote:
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.
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.
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.
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
> /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. 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
_______________________________________________
devs mailing list
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
--
Thomas Mortagne
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.
_______________________________________________
devs mailing list
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
>
>
/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/ma…
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
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
3755
days inactive
3758
days old
10 comments
6 participants
participants (6)
-
Caleb James DeLisle -
Jeremie BOUSQUET -
Marius Dumitru Florea -
Sergiu Dumitriu -
Sergiu Dumitriu -
Thomas Mortagne