11 Sep
2014
11 Sep
'14
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