Firstly, thank you very much for reviewing my code.
When I was young I hated homework, my parents told me that if I spent
as much time on the work as I spent bellyaching about it, I'd have it
done already. A lot of your review seems to take the point of view that
it's "not that hard", my counter is that no rule makes contributing
*easier* and as contributors are not exactly lining up at the door, any
rule that exists needs to justify itself as necessary.
On 09/08/2014 07:30 PM, Sergiu Dumitriu wrote:
> I'm gonna audit your audit audit.
>
> On 09/08/2014 05:14 AM, Caleb James DeLisle wrote:
>
>> /src/main/java/org.xwiki.contrib.websocket/internal/DefaultWebSocketConfig.java:26:
>> Missing a Javadoc comment.
>> Missing javadoc on a class... whose behavior is very obvious.
>> I'm not here to teach programming.
>
> Once again, I don't like "DefaultXYZ" as a name for components. Either
> only one implementation will ever make sense, in which case it probably
> shouldn't be an interface+implementation, or it would make sense to have
> alternate implementations. In the latter case, there's nothing "default"
This is the first time I've heard that. Indeed I don't like "Default"
either but it seems we don't use "Impl". This question brings up a more
difficult point, why do we need an interface for each thing we register
in the CM, even if there's only ever going to be one impl?
> about the implementation, other than it is used in the XWiki Platform as
> the default implementation. The name should mention what's specific to
> the implementation, and so should the javadoc:
>
> /** Implementation for the {@link WebSocketConfig} role which allows
> configuring the {@link XWikiWebSocketScriptService websocket service}
> through the usual XWiki configuration places: space preferences, wiki
> preferences, and {@code xwiki.properties}. */
>
> public class WikiWebSocketConfig
Although I'm tempted now to copy/paste your comment into the code, I'm
not hesitant because this is not the XWiki Configuration infra, just one
module's config.
If we hardcoded "xwiki.properties" all over the code, that would be well
accepted as a disaster, why then is it ok to hardcode documentation about
how XWiki's configuration works all over the codebase?
This class uses ConfigurationSource, whatever that might do.
>
>> /src/main/java/org.xwiki.contrib.websocket/internal/WebSocketConfig.java:55:
>> Expected an @return tag.
>>
>> I actually wrote javadoc on all of these, without even being prodded to do
>> so.
>> Do you think my freeform documentation is less useful to a reader than it
>> would be if it contained
>> a @return tag?
>
> I usually describe the meaning of the return value in the method
> javadoc, and the format / possible values / special cases in the @return
> javadoc:
>
> /**
> * The filename of the SSL private key in OpenSSL PEM format.
> *
> * @return an absolute path, or {@code null} if no certificate is
> * configured
> */
> String getPrivateKeyFilename();
>
> (Why doesn't this return a File, or an InputStream, since that's safer
> than a raw path?)
Business logic in the configuration?
Should the configuration also be responsible for checking if the file exists?
>
>> /src/main/java/org.xwiki.contrib.websocket/internal/XWikiWebSocketScriptService.java:45:
>> Missing a Javadoc comment.
>> /src/main/java/org.xwiki.contrib.websocket/internal/XWikiWebSocketScriptService.java:50:5:
>> Missing a Javadoc comment.
>> /src/main/java/org.xwiki.contrib.websocket/internal/XWikiWebSocketScriptService.java:100:5:
>> Missing a Javadoc comment.
>>
>> Do you really think that a 3 line function called getUser() needs a javadoc
>> comment?
>
> I think that getUser doesn't belong here at all. Since it just returns
Skirting the question.
Do you really think that a 3 line function called somethingOtherThanGetUser()
needs a javadoc comment?
> the current user, why is it needed? There's already
> $xcontext.userReference, and if you want to actually return a reference
> to XWikiGuest instead of null, then that's wrong and deprecated.
I do want a reference to XWikiGuest, null has a different meaning in this
context. There are of course many ways to solve a given problem but if you
claim that the implementation detail of how the internals of this class
behave are wrong and deprecated, the burden of proof is on you.
>
> As for the other longer methods, since this is a public API it should be
> documented.
>
> Also, it was agreed that script services should be in a non-internal
> org.xwiki.module.script package.
>
That's a good point, I'll document those.
>> I can guarantee you that the competition are not going to slow down and
>> smell the flowers.
>>
>>
>> /src/main/java/org.xwiki.contrib.websocket/internal/XWikiWebSocketScriptService.java:121:79:
>> The String "/" appears 4 times in the file.
>>
>> And it might appear 400 times, defining a static final string called
>> URL_SEPERATOR is
>> exercise for my fingers and your eyeballs, nothing more. If I really wanted
>> to obfuscate
>> my code, I could use an external library to make you go hunting for the
>> value of
>> YouWillNeverFindMe.SEPERATOR
>
> We can exclude certain strings from the check. We currently exclude a
> few common strings: "", "[0-9]", " ", "]". We could add "/" as well.
>
> This rule is supposed to protect us against poor-man's constants
> copy-pasted in many places, since this is error-prone (typos) and hard
> to refactor. It also forces us to define useless constants just for
> working around this rule, which I agree is almost as bad. There's a
> tradeoff between maintainability and convenience, and the XWiki
> community agreed to go for more maintainability.
It's not more maintainable, it just looks more maintainable.
If someone's hardcoding filesystem paths inside of classes then they're
certainly doing so many other horrible things that the file paths should
be preserved as a red flag for reviewers!
99% of strings are not file paths, they're error messages and forward
slashes and other things which will never change. Even configuration
keys are impossible to change once they're deployed in the field.
#define TEN 10
>
>> /src/main/java/org.xwiki.contrib.websocket/internal/WebSocketService.java:25:
>> Missing a Javadoc comment.
>> /src/main/java/org.xwiki.contrib.websocket/internal/WebSocketService.java:28:5:
>> Missing a Javadoc comment.
>>
>> It's inside of an /internal/ directory, if you can't infer it's meaning
>> then either you can't read or I can't code.
>
> It's a component role, but it's internal. That's a bit of a
> contradiction, but let's ignore that for the moment.
>
> I looked at that interface (ignoring the JavaDoc that's there now), and
> out of context I can't infer it's meaning.
Because it's internal, I assume that you can just look at the implementation.
But this gets back to the question of why we need to define interfaces for
which there will never be more than one impl.
>
>>
>> /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocket.java:33:
>> Missing a Javadoc comment.
>> /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocket.java:58:5:
>> Missing a Javadoc comment.
>> /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocket.java:59:5:
>> Missing a Javadoc comment.
>> /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocket.java:60:5:
>> Missing a Javadoc comment.
>> /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocket.java:62:5:
>> Missing a Javadoc comment.
>> /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocket.java:67:5:
>> Missing a Javadoc comment.
>> /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocket.java:69:5:
>> Missing a Javadoc comment.
>> /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocket.java:70:5:
>> Missing a Javadoc comment.
>>
>> It's internal and this module is small, if a reasonable programmer needs this
>> granularity to understand this module, it should be rewritten from scratch.
>
> Except the type, these actually are missing an @Override. As for the type:
Override indeed solves the checkstyle error.
>
> /**
> * {@link WebSocket} implementation using the
> * <a href="http://netty.io/";>Netty library</a>.
> */
Does telling the reader that Netty means netty.io make the code fundamentally
better?
If so then I could make all of my code better by telling the reader how to use
google.
>
>> /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:118:40:
>> The String "SSL enabled with websocket.ssl.certChainFile set " appears 2
>> times in the file.
>
> Easy fix:
>
> - throw new RuntimeException("SSL enabled with
> websocket.ssl.certChainFile set " +
> - "but the pkcs8PrivateKeyFile does not seem to exist.");
> + throw new RuntimeException(
> + "SSL enabled with websocket.ssl.certChainFile set but
> the pkcs8PrivateKeyFile does not seem to exist.");
>
Easy fix is still worse than "not broken", I don't want to know how easy
it is to trick this rule, I know a lot of tricks already. I want to hear
a justification for this rule in the face of the fact that it makes
development harder than it would otherwise be.
>> /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:118:92:
>> '+' should be on a new line.
>> /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:122:92:
>> '+' should be on a new line.
>> /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:132:94:
>> '+' should be on a new line.
>> /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:133:92:
>> '+' should be on a new line.
>>
>> It's arguably better to put it on the line before because some languages
>> (python)
>> infer semicolons and will fail to parse if it's on the line after.
>
> This isn't Python code. But I agree, I don't have a good reason for
> putting it on the next line, and I've wondered about this rule as well.
I write code in a number of languages and I want to adopt habits which
are as widely applicable as possible.
>
>> /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:235:9:
>> Executable statement count is 37 (max allowed is 30).
>>
>> Statements per function is almost useless as a measure of code complexity.
>> I claim negative usefulness because it is misleading.
>
> Would you say the same if the limit was at 5000 statements, and you were
> over that limit? We have to draw the line somewhere, and it was randomly
> agreed to set it at 30. This might be too low for some usecases, but in
> general it is a good enough rule IMHO. You can use your judgment and
> explicitly disable that rule for that function.
>
> But looking at the code, I think it could use some simplifying, I find
> it rather long.
>
> The rule isn't just about code complexity, but the attention span of a
> developer reading that code. How long does it take to put that whole
> method in the brain? Can most developers even fit that much code in
> memory? If not, then the method must be simplified / split.
I've written functions much longer and much easier to read as well as much
shorter functions which, due to the complexity of the algorithm, push my
own mental capacity to it's limit.
Surely I still have much room for improvement in code craftsmanship but
I don't think a checkstyle failure is any more likely to make me suddenly
discover a clearer way to express a function any more than a rejected paper
would suddenly make me a mathematician.
On the contrary, fixing these types of errors will almost certainly make
the code *worse* because the original state of the function is an expression
of the mental model of the author and in response to failures, he will break
it up along arbitrary lines in order to make the errors stop, but the code
and the programmer will not and cannot be fundamentally better than they
were 20 minutes before.
>
>>
>> /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:246:58:
>> The String "?k=" appears 2 times in the file.
>>
>> And you don't need to look anything up to understand that line of code, any
>> change would be for the worse.
>
> String key = StringUtils.substringAfter(uri, "?k=");
> uri = StringUtils.substringBefore(uri, "?");
I don't want to see the tricks, I want to see compelling justification for
the rule.
>
> (Why would any serious library use strings for representing URIs instead
> of java.net.URI?)
If I gave you a string and a handful of URI manipulating functions then even
if you have no knowledge of those functions, you can still understand it as
a plain old string, if I give you a java.net.URI then you have to go look up
API documentation no matter what, an obvious cost increase.
If there are no solid justifications for this cost then I call overengineering.
>
>> /src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java:300:38:
>> 'nws' hides a field.
>>
>> I always use 'this.' prefix when accessing object fields, I would prefer that
>> as a rule but in the absence of that rule, this one is fairly important.
>
> That's not an absence, the "always use this." rule is there.
It's not enforced by checkstyle, ftr this is one rule which I would like to see
added (but only if we can at least balance the development pain budget, if not
lower it).
>
> Having both rules is a bit redundant, since the this rule is supposed to
> allow the field hiding that the second rule is complaining about.
>
> Still, why do you need to declare nws instead of assigning to this.nws?
> As I can see, there's no exception that could stop the method between
> the nwsLocal declaration and the this.nws assignment.
>
> And I think you just introduced a bug,
> https://github.com/xwiki-contrib/xwiki-contrib-websocket/blob/master/src/main/java/org.xwiki.contrib.websocket/internal/NettyWebSocketService.java#L313
> now references "nws" which became this.nws instead of nwsLocal.
>
It will "probably never happen" but I fixed it for the next version, thanks for
spotting that. Since it's used in an inner class, I though I needed it to be
final but indeed using the field works, fixed.
So to summarize, most painful rules:
1. Multiple identical string constants in a file
2. Javadoc /internal/ classes and methods
3. (not a checkstyle rule) interfaces with one impl.
4. There are 3 or 4 rules which force you to completely re-factor a function or
class, every time one of these trips and the developer "fixes it", we're
creating
bad code.
Thanks,
Caleb
_______________________________________________
devs mailing list
[email protected]
http://lists.xwiki.org/mailman/listinfo/devs
