Critical code review of WAMP/Autobahn/Crossbar

[Cal Leeming]

Hello,

I have drafted a code review of Autobahn and Crossbar, and would welcome any feedback/corrections prior to being published.

This review was initially done in Dec 2014, but has been updated to reflect recent changes;

https://github.com/foxx/foxx.github.io/blob/master/_posts/draft-2014-12-30-crossbar-io-code-review.draft.markdown

As stated in the article, I have a lot of respect for what you guys are trying to achieve, and it's only fair I give you the opportunity to address some of these concerns prior to publishing.

Cal

[Emile Cormier]

Cal,

You claim that:

WAMP specification as a whole feels unviable

Care to elaborate?

Cheers,

Emile

[Cal Leeming]

This is based on the summary that WAMP feels over-engineered, similar
to that of RFC 3261 (SIP). The specification makes for difficult
reading, some places have nested JSON encoded as a string [1] (wtf?),
and throughout the spec there is type hinting appended to message
definition keys (|string, |bool etc). There has also been minimal
external input and peer review, the majority of the spec has been
written by one person and less than a handful of people had any deep
involvement on the group mailing list throughout its conception.

Furthermore, WAMP is a trademark which belongs to the company who has
sponsored its development, and it uses the CC license which is not
recommended for software and is not GPL compatible. Also many of the
Autobahn WAMP family use differing licenses, Apache/MIT etc.

Cal

[1]: https://github.com/tavendo/WAMP/blob/master/spec/advanced.md#password-salting

> --
> You received this message because you are subscribed to a topic in the
> Google Groups "Autobahn" group.
> To unsubscribe from this topic, visit
> https://groups.google.com/d/topic/autobahnws/EMKF4a0aXKI/unsubscribe.
> To unsubscribe from this group and all its topics, send an email to
> autobahnws+...@googlegroups.com.
> To post to this group, send email to autob...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/autobahnws/af198ade-47ae-41ee-b2aa-7453cfdcea47%40googlegroups.com.
>
> For more options, visit https://groups.google.com/d/optout.

[Emile Cormier]

Cal,

Thank you for elaborating on that particular opinion. I'd like to point that the WAMP spec is a document, and not software, so I don't understand your concerns regarding the CC license.

Emile

[Cal Leeming]

Based on my interpretation of the licenses, the specification is
closely related to software and as such not very suitable. However
legal/licenses are not my strongest point so, if I have misinterpreted
this then do let me know.

Cal

> --
> You received this message because you are subscribed to a topic in the
> Google Groups "Autobahn" group.
> To unsubscribe from this topic, visit
> https://groups.google.com/d/topic/autobahnws/EMKF4a0aXKI/unsubscribe.
> To unsubscribe from this group and all its topics, send an email to
> autobahnws+...@googlegroups.com.
> To post to this group, send email to autob...@googlegroups.com.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/autobahnws/b718887f-93c6-4659-b213-5fd3ec8551d4%40googlegroups.com.

[Emile Cormier]

On Wednesday, June 10, 2015 at 5:06:11 PM UTC-3, Cal Leeming wrote:

Based on my interpretation of the licenses, the specification is
closely related to software and as such not very suitable. However
legal/licenses are not my strongest point so, if I have misinterpreted
this then do let me know.

Cal

The WAMP specification document has vastly different distribution concerns than software implementations. Even the Free Software Foundation recognised this, so they provide the GNU Free Documentation License.

If the WAMP were released under, say, the GPL, there would be nothing preventing someone selling millions of printed copies of the spec for a profit. This blog article discusses reasons why you'd want external documentation licenses to be different than the source code license.

[Emile Cormier]

On Wednesday, June 10, 2015 at 4:40:38 PM UTC-3, Cal Leeming wrote:

This is based on the summary that WAMP feels over-engineered, similar
to that of RFC 3261 (SIP).

Please don't take this the wrong way, but how you "feel" about the spec is a rather weak subjective argument. And I don't see how SIP has anything to do WAMP. Exactly what features of WAMP do you think are superfluous?

 

the majority of the spec has been
written by one person and less than a handful of people had any deep
involvement on the group mailing list throughout its conception.

The quality of a communications protocol is not a function of the number of people who wrote it. CORBA, for instance, was designed by committee, and look where it is now.

[Cal Leeming]

Thanks for the feedback, I'll include a link to this thread in the article.

I was also not aware of the concerns regarding external documentation
licenses, and will update the article accordingly. You also make a
good point in regards to being designed by committee, but this doesn't
excuse the lack of peer review.

I appreciate your comments about it being a weak argument, but will
politely disagree. The purpose of this article is that of a code
review, rather than suggesting ways that the WAMP specification itself
could be improved, as personally I feel that WAMP as a whole is not
the right way forward. However as you note, I've made it very clear
that this is a personal opinion, and I have urged people to review the
project for themselves so they can come to their own conclusions.

To clarify my points about SIP, I did not state that the two were
linked in any way, I said that it reminded me of SIP, in that it feels
over-engineered and hard on the eyes.

If you have any comments/feedback on the code review itself, which is
the primary focus of the article, please do feel free to respond with
these too.

Cal

> --
> You received this message because you are subscribed to a topic in the
> Google Groups "Autobahn" group.
> To unsubscribe from this topic, visit
> https://groups.google.com/d/topic/autobahnws/EMKF4a0aXKI/unsubscribe.
> To unsubscribe from this group and all its topics, send an email to
> autobahnws+...@googlegroups.com.
> To post to this group, send email to autob...@googlegroups.com.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/autobahnws/99e4d95a-e715-4f70-a687-9fd558bfb30a%40googlegroups.com.

[Emile Cormier]

On Wednesday, June 10, 2015 at 6:29:22 PM UTC-3, Cal Leeming wrote:

I appreciate your comments about it being a weak argument, but will
politely disagree. The purpose of this article is that of a code
review, rather than suggesting ways that the WAMP specification itself
could be improved, as personally I feel that WAMP as a whole is not
the right way forward. However as you note, I've made it very clear
that this is a personal opinion, and I have urged people to review the
project for themselves so they can come to their own conclusions.

I was actually trying to help you with those comments. In Essay Writing 101, you learn that broad, one-sentence opinions without any supporting facts are essentially worthless. Your readers will have the same reaction that I did.

If the purpose of the article is a code review of two particular WAMP implementations, then I suggest that you leave out blanket statements regarding the WAMP spec if you're not prepared to back up your claims.

 

To clarify my points about SIP, I did not state that the two were
linked in any way, I said that it reminded me of SIP, in that it feels
over-engineered and hard on the eyes.

If you have any comments/feedback on the code review itself, which is
the primary focus of the article, please do feel free to respond with
these too.

I am not an expert in Javascript or Python, so I cannot provide any feedback on your code review. I am, however, intimately familiar with the WAMP specification, having implemented a client library in C++. I have not found any of the stable or semi-stable WAMP features to be over-engineered or superfluous. I can easily picture how they would be useful in a distributed web application. That is why I "jumped" all over your statement that the WAMP protocol is not "viable" and "over-engineered".

The author(s) of Crossbar and Autobahn|JS might have rebuttals for your code critiques, however.

[Cal Leeming]

On Wed, Jun 10, 2015 at 10:59 PM, Emile Cormier
<emile.co...@gmail.com> wrote:
> On Wednesday, June 10, 2015 at 6:29:22 PM UTC-3, Cal Leeming wrote:
>>
>> I appreciate your comments about it being a weak argument, but will
>> politely disagree. The purpose of this article is that of a code
>> review, rather than suggesting ways that the WAMP specification itself
>> could be improved, as personally I feel that WAMP as a whole is not
>> the right way forward. However as you note, I've made it very clear
>> that this is a personal opinion, and I have urged people to review the
>> project for themselves so they can come to their own conclusions.
>
>
> I was actually trying to help you with those comments. In Essay Writing 101,
> you learn that broad, one-sentence opinions without any supporting facts are
> essentially worthless. Your readers will have the same reaction that I did.

Please don't take my response as a negative/rejection, I appreciate
all feedback and respect that everyone is entitled to an opinion, and
your comments/suggestions are very much welcomed, regardless of
whether I agree with them or not.

I had a similar response from the Docker founders when I criticised
their product as a whole too, yet many of the readers felt compelled
to agree with my comments, despite the project maintainers stating
that my claims were unfounded. I could spend a week going over the
specification, debating every component and coming up with a better
solution, but that's not my primary concern. At the very least, my
comments on WAMP can be considered subjective.

>
> If the purpose of the article is a code review of two particular WAMP
> implementations, then I suggest that you leave out blanket statements
> regarding the WAMP spec if you're not prepared to back up your claims.

I have backed up my claims, whether you agree with them or feel they
are substantial enough is a different matter. For the most part, it
comes down to a matter of taste, which can be a difficult thing to
explain to someone when they are on a very different wavelength.

>
>>
>>
>> To clarify my points about SIP, I did not state that the two were
>> linked in any way, I said that it reminded me of SIP, in that it feels
>> over-engineered and hard on the eyes.
>>
>> If you have any comments/feedback on the code review itself, which is
>> the primary focus of the article, please do feel free to respond with
>> these too.
>
>
> I am not an expert in Javascript or Python, so I cannot provide any feedback
> on your code review. I am, however, intimately familiar with the WAMP
> specification, having implemented a client library in C++. I have not found
> any of the stable or semi-stable WAMP features to be over-engineered or
> superfluous. I can easily picture how they would be useful in a distributed
> web application. That is why I "jumped" all over your statement that the
> WAMP protocol is not "viable" and "over-engineered".
>
> The author(s) of Crossbar and Autobahn|JS might have rebuttals for your code
> critiques, however.
>

> --
> You received this message because you are subscribed to a topic in the
> Google Groups "Autobahn" group.
> To unsubscribe from this topic, visit
> https://groups.google.com/d/topic/autobahnws/EMKF4a0aXKI/unsubscribe.
> To unsubscribe from this group and all its topics, send an email to
> autobahnws+...@googlegroups.com.
> To post to this group, send email to autob...@googlegroups.com.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/autobahnws/2cc5e301-039b-4515-8929-5e2ac54256df%40googlegroups.com.

[Tobias Oberstein]

Hi Cal,

your "review" is largely just personal opinion. As such, that is fine,
but IMO, you should make it more clear to the reader that it's your
personal opinion here, not a thorough technical review (which it is not).

Let me just address one aspect of your text that _appears_ to have some
backing in arguments: code quality.

AutobahnPython/Crossbar.io: each an every commit is automatically built
on all supported platforms (Travis), tested with an automated set of
unit tests (Tox/Coverage) on all supported platforms, as well as
validated for PEP8 code compliance (Flake8).

Also, you might note that http://autobahn.ws/testsuite/ (which is based
on AutobahnPython) is used by nearly all the industry to check their
WebSocket implementations. If our code quality was that bad, that
wouldn't be the case.

Cheers,
/Tobias

>
> Cal
>

[Cal Leeming]

On Thu, Jun 11, 2015 at 1:06 PM, Tobias Oberstein
<tobias.o...@gmail.com> wrote:
> Am 10.06.2015 um 14:49 schrieb Cal Leeming:
>> Hello,
>>
>> I have drafted a code review of Autobahn and Crossbar, and would welcome
>> any feedback/corrections prior to being published.
>>
>> This review was initially done in Dec 2014, but has been updated to reflect
>> recent changes;
>> https://github.com/foxx/foxx.github.io/blob/master/_posts/draft-2014-12-30-crossbar-io-code-review.draft.markdown
>>
>> As stated in the article, I have a lot of respect for what you guys are
>> trying to achieve, and it's only fair I give you the opportunity to address
>> some of these concerns prior to publishing.
>
> Hi Cal,
>
> your "review" is largely just personal opinion. As such, that is fine,
> but IMO, you should make it more clear to the reader that it's your
> personal opinion here, not a thorough technical review (which it is not).

Thank you for your feedback.

Although the comments relating to the WAMP spec are subjective, the
code review is an entirely technical and quite thorough.

There are legitimate concerns raised about code quality, which so far
no one has offered up any rebuttal for (other than completely
disregarding them).

Am I correct in saying that you feel this code is of an acceptable
quality and production worthy? Could you give your thoughts on some of
the technical concerns raised in both the py/js implementations?

>
> Let me just address one aspect of your text that _appears_ to have some
> backing in arguments: code quality.
>
> AutobahnPython/Crossbar.io: each an every commit is automatically built
> on all supported platforms (Travis), tested with an automated set of
> unit tests (Tox/Coverage) on all supported platforms, as well as
> validated for PEP8 code compliance (Flake8).

There are several PEP8 violations, which were already mentioned, and
just because the code passes some tests does not mean the quality is
good.

>
> Also, you might note that http://autobahn.ws/testsuite/ (which is based
> on AutobahnPython) is used by nearly all the industry to check their
> WebSocket implementations. If our code quality was that bad, that
> wouldn't be the case.

If you read again, you'll see that I actually added a positive note
about the test suite, and that it was used by many industry leaders.

However, this fact alone does not automatically grant you the status
of having good code quality, it simply means you have built a set of
tests which accurately test WS implementation. The python/js libs are
a completely different story, and the number of people using your test
suite has no bearing on code quality.

>
> Cheers,
> /Tobias
>
>>
>> Cal

>>
>
> --
> You received this message because you are subscribed to a topic in the Google Groups "Autobahn" group.
> To unsubscribe from this topic, visit https://groups.google.com/d/topic/autobahnws/EMKF4a0aXKI/unsubscribe.
> To unsubscribe from this group and all its topics, send an email to autobahnws+...@googlegroups.com.
> To post to this group, send email to autob...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/autobahnws/557979AA.2060109%40gmail.com.

[Tobias Oberstein]

>> your "review" is largely just personal opinion. As such, that is fine,
>> but IMO, you should make it more clear to the reader that it's your
>> personal opinion here, not a thorough technical review (which it is not).
>
> Thank you for your feedback.
>
> Although the comments relating to the WAMP spec are subjective, the
> code review is an entirely technical and quite thorough.

I reread it, and I can't find a single issue that is valid or still
applies. Can you point me to one code quality issue regarding Python in
AutobahnPython or Crossbar.io?

Cheers,
Tobias

[Michel Desmoulin]

I took the time to read the entire review.

Some claims are valid : documentation needs improvements, the JS API too.

But the conclusion that the global code quality is bad is extremly exagerated. I see thousands of lines of code everyday that are not remotly as good as the crossbar code and run in prod, serving users and making money.

Not to say the code quality could not be improved : chained if are a problem, comments could be better, and the crossbar configuration system is very inflexible (as you said, blog of JS, and no way to configure that manually in python).

Still, jumping to the conclusion that you should not use this software for that is crazy. Otherwise, I hope you coded your own OS cause i never heard of one with only tools using the code quality you expect.

I still think your review is valuable, and the team should swallow the pill and work on it (note they have limited ressource though), yet it's harsh, unrealistic and alarmist.

[Cal Leeming]

Throughout the review there are specific instances of bad code within
Autobahn JS and Crossbar, each with a link to the line/commit on
Github, plus comments (some positive) on various aspects of the
project, including Autobahn Python.

At this point I'd just repeating what is already in the article, it's
up to others to come to their own conclusion now.

Cal

> --
> You received this message because you are subscribed to a topic in the
> Google Groups "Autobahn" group.
> To unsubscribe from this topic, visit
> https://groups.google.com/d/topic/autobahnws/EMKF4a0aXKI/unsubscribe.
> To unsubscribe from this group and all its topics, send an email to
> autobahnws+...@googlegroups.com.
> To post to this group, send email to autob...@googlegroups.com.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/autobahnws/5579827C.1080308%40gmail.com.

[Cal Leeming]

Thank you for your feedback, this is precisely the sort of input that
I was looking for, and the points you have made are indeed valid.

I have purposely urged others to come to their own conclusion, and
have included a link to a positive review of Crossbar at the very top
of the article, as well as some positive points from myself.

I'm hoping that my thoughts/comments will be taken on board and that
it will result in some code refactoring, as I do admire what this
project is trying to achieve.

Cal

> --
> You received this message because you are subscribed to a topic in the
> Google Groups "Autobahn" group.
> To unsubscribe from this topic, visit
> https://groups.google.com/d/topic/autobahnws/EMKF4a0aXKI/unsubscribe.
> To unsubscribe from this group and all its topics, send an email to
> autobahnws+...@googlegroups.com.
> To post to this group, send email to autob...@googlegroups.com.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/autobahnws/145a3c85-4d6b-4ed1-81ec-ab35bd478621%40googlegroups.com.

[David Dan]

Man, I sure hope that WAMP doesn't do as well Docker.  That would be a tragedy.  I mean, all those poor people out there that don't know that Docker "is no where near production ready".  

Seriously, this guy does raise a couple of good points (all projects can be improved), but overall, he doesn't rise above the level of troll.

[Cal Leeming]

Thank you for the honest feedback, regardless of the sarcasm, it is appreciated.

Cal

On Thursday, June 11, 2015, David Dan <davi...@gmail.com> wrote:

Man, I sure hope that WAMP doesn't do as well Docker.  That would be a tragedy.  I mean, all those poor people out there that don't know that Docker "is no where near production ready".  

Seriously, this guy does raise a couple of good points (all projects can be improved), but overall, he doesn't rise above the level of troll.

--

You received this message because you are subscribed to a topic in the Google Groups "Autobahn" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/autobahnws/EMKF4a0aXKI/unsubscribe.
To unsubscribe from this group and all its topics, send an email to autobahnws+...@googlegroups.com.
To post to this group, send email to autob...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/autobahnws/eea6063e-ec32-46d2-9752-01ca01851d5b%40googlegroups.com.

[David Zentgraf]

Hi Cal,

As a critical review of your critical review, I would concur with others that it's also lacking a bit. I certainly appreciate criticism as much as the next guy, especially since I'm in the middle of building a product with Crossbar. However, your critique lacks heft for its conclusion to be taken too seriously. I understand you set a short time limit for this survey, however that fact also doesn't help the cause.

You do not cite any specific test setup for your performance test, nor whether the high CPU usage is a result of Autobahn or other factors. The entire paragraph lacks substance and validity. Further, it's about example code, not production code.

You criticise the "Getting started" docs for "I don't see any documentation on what each attribute does". I do believe all options are documented somewhere, though not necessarily all within the Getting started page. Are you saying you have not found any documentation for the configuration anywhere, or that you simply didn't dig deep enough?

Many links to specific code are 404. I don't know if this changed within the last day or what. Inline code samples would make for a stronger case.

As others have noted, your critique of the spec is very vague. More in-depth examples of what "over engineered" means or samples of how a similar protocol might be implemented in a leaner fashion would help your case.

I do think there are valid criticisms of some code quality samples; though they do not point towards any concrete problems with how the code behaves in production.

In the context of a severely resource constrained open source project, most of your criticism seems to focus on minor aspects which are sort of expected. I suppose if resource constrains, documentation issues and polish are a deal breaker for you in deciding about a technology, this is certainly fair criticism. However, using this as grounds for declaring the entire project "unviable" is a bit thin.

If anything, you would need to specify your requirements and goals and the context in which the project is unviable *for you*. I can certainly see why a corporate entity or such would not choose Crossbar/Autobahn at the moment. Your review could be much more valid if you'd establish some initial parameters for your review.

Best,

Dav

On Wednesday, June 10, 2015 at 2:49:21 PM UTC+2, Cal Leeming wrote:

[Cal Leeming]

Thank you for the in-depth feedback, you have raised some excellent points.

(I'll post back with a commit diff once done)

On Fri, Jun 12, 2015 at 9:20 AM, David Zentgraf <da...@spinshell.com> wrote:
> Hi Cal,
>
> As a critical review of your critical review, I would concur with others
> that it's also lacking a bit. I certainly appreciate criticism as much as
> the next guy, especially since I'm in the middle of building a product with
> Crossbar. However, your critique lacks heft for its conclusion to be taken
> too seriously. I understand you set a short time limit for this survey,
> however that fact also doesn't help the cause.

Admittedly, the time limit has reduced the overall quality.

>
> You do not cite any specific test setup for your performance test, nor
> whether the high CPU usage is a result of Autobahn or other factors. The
> entire paragraph lacks substance and validity. Further, it's about example
> code, not production code.

The high CPU was in relation to browser usage with Autobahn JS, which
anyone can reproduce using the demo page in their browser. However I
only tested with Chrome, so I will try a few other browsers and update
the article to reflect this.

>
> You criticise the "Getting started" docs for "I don't see any documentation
> on what each attribute does". I do believe all options are documented
> somewhere, though not necessarily all within the Getting started page. Are
> you saying you have not found any documentation for the configuration
> anywhere, or that you simply didn't dig deep enough?

On first inspection, many of the class attributes/methods were
undocumented, however I will update the article to give some examples
of this.

>
> Many links to specific code are 404. I don't know if this changed within the
> last day or what. Inline code samples would make for a stronger case.

Apologies, the article was originally written 6 months ago and the
crossbar demo URLs seem to have changed. I also accidentally linked to
the master branch, rather than the release branch, which didn't help,
although this wouldn't have helped in this case as the entire original
repo was moved to "crossbarexamples".

I also did not include my reasoning behind why I felt the demo code
was of poor quality, and will update the article to reflect this.

>
> As others have noted, your critique of the spec is very vague. More in-depth
> examples of what "over engineered" means or samples of how a similar
> protocol might be implemented in a leaner fashion would help your case.

This is a good point. So far I am yet to find any other system which
offers, at least what I would consider, a clean approach. As such it
is difficult to give direct comparisons on how the project as a whole
could be done better, as there isn't much else to compare it to (any
suggestions?)

However I should have provided a better explanation as to why I felt
it was over-engineered, and will update to reflect.

>
> I do think there are valid criticisms of some code quality samples; though
> they do not point towards any concrete problems with how the code behaves in
> production.
> In the context of a severely resource constrained open source project, most
> of your criticism seems to focus on minor aspects which are sort of
> expected. I suppose if resource constrains, documentation issues and polish
> are a deal breaker for you in deciding about a technology, this is certainly
> fair criticism. However, using this as grounds for declaring the entire
> project "unviable" is a bit thin.

I'll update with some examples of clean code by unrelated libraries in
their respective language, with similar team/contributor sizes.

Side note - (imho) it doesn't matter if you have 1 person or 100
people, clean code is within the reach of every project. Having
limited contributors/resources is absolutely no reason to sacrifice
clean code, although this is a wider picture debate that deserves a
whole article for itself. It's also a topic that has historically
divided people, everyone has different emotions towards code, some see
it as a business tool, others see it as an art form, some are a mix.

>
> If anything, you would need to specify your requirements and goals and the
> context in which the project is unviable *for you*. I can certainly see why
> a corporate entity or such would not choose Crossbar/Autobahn at the moment.
> Your review could be much more valid if you'd establish some initial
> parameters for your review.

Fair point, I will update to reflect.

>
> Best,
> Dav
>
>
>
> On Wednesday, June 10, 2015 at 2:49:21 PM UTC+2, Cal Leeming wrote:
>>
>> Hello,
>>
>> I have drafted a code review of Autobahn and Crossbar, and would welcome
>> any feedback/corrections prior to being published.
>>
>> This review was initially done in Dec 2014, but has been updated to
>> reflect recent changes;
>>
>> https://github.com/foxx/foxx.github.io/blob/master/_posts/draft-2014-12-30-crossbar-io-code-review.draft.markdown
>>
>> As stated in the article, I have a lot of respect for what you guys are
>> trying to achieve, and it's only fair I give you the opportunity to address
>> some of these concerns prior to publishing.
>>
>> Cal
>

> --
> You received this message because you are subscribed to a topic in the
> Google Groups "Autobahn" group.
> To unsubscribe from this topic, visit
> https://groups.google.com/d/topic/autobahnws/EMKF4a0aXKI/unsubscribe.
> To unsubscribe from this group and all its topics, send an email to
> autobahnws+...@googlegroups.com.
> To post to this group, send email to autob...@googlegroups.com.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/autobahnws/42c40edd-ab42-43a3-b732-297423aa574e%40googlegroups.com.

[David C. Zentgraf]

> On 12 Jun 2015, at 12:18, Cal Leeming <c...@iops.io> wrote:
>
> Apologies, the article was originally written 6 months ago

Arguably Crossbar has come quite some ways in the last 6 months.
Perhaps an entire re-evaluation is in order.

> Side note - (imho) it doesn't matter if you have 1 person or 100
> people, clean code is within the reach of every project. Having
> limited contributors/resources is absolutely no reason to sacrifice

> clean code …

I surely agree here, code quality should certainly always be a high priority. Virtually all code could virtually always be better. The pertinent questions are: how unreasonable is the code quality of Crossbar/Autobahn really, and how much does that really impact you as the user of the product?

Best,
Dav

[Tobias Oberstein]

Cal,

since your text has triggered some responses, I'll now take the time and
will prove it wrong on each "factual" point you raise.

I will limit myself to AutobahnPython and Crossbar.io

Hopefully, this will then spare us all some time.

>> As a critical review of your critical review, I would concur with others
>> that it's also lacking a bit. I certainly appreciate criticism as much as
>> the next guy, especially since I'm in the middle of building a product with
>> Crossbar. However, your critique lacks heft for its conclusion to be taken
>> too seriously. I understand you set a short time limit for this survey,
>> however that fact also doesn't help the cause.
>
> Admittedly, the time limit has reduced the overall quality.

No, it wasn't a thorough review at any time. However, as of today, it's
factually wrong in most points.

So your code review of a approx. 40k LOC code base (AutobahnPython and
Crossbar.io) consists of 5 paragraphs here

https://github.com/foxx/foxx.github.io/blob/master/_posts/draft-2014-12-30-crossbar-io-code-review.draft.markdown#python-server-code

> Initial docs inspection worries me, their getting started docs
appears to show a blob of JSON which is presumably used to configure the
application,

Wrong:
There is no JSON on http://crossbar.io/docs/Getting-started-with-Python/

Invalid:
This is about docs, not code quality.

> There is also a section on "hacking code" just underneath, which
doesn't give any explanation on what it does and why it would be necessary.

Wrong:
The example code on that page is commented, giving hints on what each
block does.

> their repo is immediately untidy from the outset with an older
version of the code rotting away in a folder called v1.

Wrong:
The v1 code has been removed end of 2014.

Invalid:
This isn't about code quality, but organization.

> Makefile seems to contain several commands with no explanation on
what each one does, and some rather strange attribute names such as cbdir

Wrong:
The Makefile contains a description of make targets:
https://github.com/crossbario/crossbar/blob/master/Makefile#L6

Invalid:
Weather some Makefile targets are documented or not is an arbitrary,
random point weakly realted to code quality.

> In some cases, paths have been hard coded to a developers local setup
Makefile#L70

Wrong:
There aren't any hard-coded paths in
https://github.com/crossbario/crossbar/blob/master/Makefile

Invalid:
So you are looking for hard-coded paths in your "code quality
assessment"? ;)

> Most files have the same license text unnecessarily repeated at the top

Invalid:
This is a legal requirement.

> Many of the PEP8 guidelines have not been followed

Wrong:
We have a full set of automated PEP8 tests running on each commit. The
code is 100% PEP8 (other than variable naming and long lines).

> Some parts of the code have a good amount of class documentation
publisher.py#L39, but the majority does not guest.py#L123.

Invalid:
What you have looked at are _internal_ classes. Internal classes should
have documentation in code comments, but not API documentation.

> Many sections have nested conditionals which look particularly ugly
publisher.py#L93, and this seems to be repeated throughout the code
broker.py#L170.

Invalid:
This is your personal taste, but simply the required control flow in
these methods.

===

In valid review would have at least touched on the aspects of testing!
Units tests, coverage, integration tests, ..

You would have touched aspects like outside contributions, number of
open/closed issues, time to resolution, etc etc

I could go on. I guess any seasoned Python developer would expect _much_
more from a text that claims to "do a thorough code review" of a 40k LOC
code base.

==

Anyway. Please all: let's move on and stop wasting time.

Cheers,
/Tobias

>
>>
>> You do not cite any specific test setup for your performance test, nor
>> whether the high CPU usage is a result of Autobahn or other factors. The
>> entire paragraph lacks substance and validity. Further, it's about example
>> code, not production code.
>
> The high CPU was in relation to browser usage with Autobahn JS, which
> anyone can reproduce using the demo page in their browser. However I
> only tested with Chrome, so I will try a few other browsers and update
> the article to reflect this.

As said, the CPU load is due to _jQuery Sliders_, not AutobahnJS.

Your wording suggests CPU load to be an issue of AutobahnJS or WAMP -
which it is not.

[Cal Leeming]

I can't seem to find the original docs page, and would appear to have
been changed since I wrote this (another user stated that Crossbar has
changed a lot in the last 6 months?). The current demo looks better,
from a front standpoint at least.

>
>> There is also a section on "hacking code" just underneath, which doesn't
>> give any explanation on what it does and why it would be necessary.
>
> Wrong:
> The example code on that page is commented, giving hints on what each block
> does.
>
>> their repo is immediately untidy from the outset with an older version of
>> the code rotting away in a folder called v1.
>
> Wrong:
> The v1 code has been removed end of 2014.
>
> Invalid:
> This isn't about code quality, but organization.

That's a tedious argument at best, the two go hand in hand.

>
>> Makefile seems to contain several commands with no explanation on what
>> each one does, and some rather strange attribute names such as cbdir
>
> Wrong:
> The Makefile contains a description of make targets:
> https://github.com/crossbario/crossbar/blob/master/Makefile#L6
>
> Invalid:
> Weather some Makefile targets are documented or not is an arbitrary, random
> point weakly realted to code quality.

Completely disagree, how do you expect new contributors to come along
and automatically understand what each make target does? There should
be in-line comments for targets which are not immediately obvious.

>
>> In some cases, paths have been hard coded to a developers local setup
>> Makefile#L70
>
> Wrong:
> There aren't any hard-coded paths in
> https://github.com/crossbario/crossbar/blob/master/Makefile

There was, but this has since been fixed it would seem.

>
> Invalid:
> So you are looking for hard-coded paths in your "code quality assessment"?
> ;)

Are you saying that having developers local paths in an open source
project Makefile does not fall under the category of poor code
quality? Seriously?

>
>> Most files have the same license text unnecessarily repeated at the top
>
> Invalid:
> This is a legal requirement.
>
>> Many of the PEP8 guidelines have not been followed
>
> Wrong:
> We have a full set of automated PEP8 tests running on each commit. The code
> is 100% PEP8 (other than variable naming and long lines).

Variable naming is a hugely important part of clean code, as are long
lines. I'm actually due to update the article with more criticisms
about poor naming convention, as it is worse than I had initially
seen.

>
>> Some parts of the code have a good amount of class documentation
>> publisher.py#L39, but the majority does not guest.py#L123.
>
> Invalid:
> What you have looked at are _internal_ classes. Internal classes should have
> documentation in code comments, but not API documentation.

I'm not sure I understand your response, what do you mean by API documentation?

The issue I raised is quite simple, some classes have good
documentation (as code comments), others do not.

>
>> Many sections have nested conditionals which look particularly ugly
>> publisher.py#L93, and this seems to be repeated throughout the code
>> broker.py#L170.
>
> Invalid:
> This is your personal taste, but simply the required control flow in these
> methods.

Disagreed, it can be done so much better.

>
> ===
>
> In valid review would have at least touched on the aspects of testing! Units
> tests, coverage, integration tests, ..
>
> You would have touched aspects like outside contributions, number of
> open/closed issues, time to resolution, etc etc
>
> I could go on. I guess any seasoned Python developer would expect _much_
> more from a text that claims to "do a thorough code review" of a 40k LOC
> code base.

Given the time limitations, and the fact I had already made a
conclusion decision to say no, it did not make any sense to dive
deeper.

>
> ==
>
> Anyway. Please all: let's move on and stop wasting time.

The fact that you see discussing ways to improve code quality as
"wasting time" speak volumes in itself.

Naturally I thank you for your input, and will update the article to
reflect some of the points you have made, but we are on two /very/
different wavelengths.

>
> Cheers,
> /Tobias
>
>
>>
>>>
>>> You do not cite any specific test setup for your performance test, nor
>>> whether the high CPU usage is a result of Autobahn or other factors. The
>>> entire paragraph lacks substance and validity. Further, it's about
>>> example
>>> code, not production code.
>>
>>
>> The high CPU was in relation to browser usage with Autobahn JS, which
>> anyone can reproduce using the demo page in their browser. However I
>> only tested with Chrome, so I will try a few other browsers and update
>> the article to reflect this.
>
>
> As said, the CPU load is due to _jQuery Sliders_, not AutobahnJS.
>
> Your wording suggests CPU load to be an issue of AutobahnJS or WAMP - which
> it is not.
>
>

> --
> You received this message because you are subscribed to a topic in the
> Google Groups "Autobahn" group.
> To unsubscribe from this topic, visit
> https://groups.google.com/d/topic/autobahnws/EMKF4a0aXKI/unsubscribe.
> To unsubscribe from this group and all its topics, send an email to
> autobahnws+...@googlegroups.com.
> To post to this group, send email to autob...@googlegroups.com.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/autobahnws/557AD2B1.3020500%40gmail.com.

[Raito Bezarius]

Le vendredi 12 juin 2015 14:56:24 UTC+2, Cal Leeming a écrit 

I disagree entirely.

Seriously, Crossbar.io's Makefile is extremely simple and terse. It *is* obvious in this case.

>
>> In some cases, paths have been hard coded to a developers local setup
>> Makefile#L70
>
> Wrong:
> There aren't any hard-coded paths in
> https://github.com/crossbario/crossbar/blob/master/Makefile

There was, but this has since been fixed it would seem.

>
> Invalid:
> So you are looking for hard-coded paths in your "code quality assessment"?
> ;)

Are you saying that having developers local paths in an open source
project Makefile does not fall under the category of poor code
quality? Seriously?

He didn't say that. 

>
>> Most files have the same license text unnecessarily repeated at the top
>
> Invalid:
> This is a legal requirement.
>
>> Many of the PEP8 guidelines have not been followed
>
> Wrong:
> We have a full set of automated PEP8 tests running on each commit. The code
> is 100% PEP8 (other than variable naming and long lines).

Variable naming is a hugely important part of clean code, as are long
lines. I'm actually due to update the article with more criticisms
about poor naming convention, as it is worse than I had initially
seen.

As far as I know and I recall, PEP8 is not able to understand if a variable has a *bad* name, except if it is obvious.

I said a bad name in that this name will hurt the code quality.

I didn't say a bad name in that this name will not follow a fixed and strict coding style.

Do you have an example of poor naming convention in WAMP which hurts the code quality ?

>
>> Some parts of the code have a good amount of class documentation
>> publisher.py#L39, but the majority does not guest.py#L123.
>
> Invalid:
> What you have looked at are _internal_ classes. Internal classes should have
> documentation in code comments, but not API documentation.

I'm not sure I understand your response, what do you mean by API documentation?

API is what it is exposed publicly, to users, to clients. So what do matters to Autobahn's users / Crossbar's users in most of the cases.

It is that the API is documented. I may agree that the documentation may be bad sometimes, some behaviors are not sufficiently documented.

But internal classes has nothing to do with documentation, it is to the discretion of contributors to decide if they want a documentation or not.

The issue I raised is quite simple, some classes have good
documentation (as code comments), others do not.

Because some classes need documentation and others don't. 

>
>> Many sections have nested conditionals which look particularly ugly
>> publisher.py#L93, and this seems to be repeated throughout the code
>> broker.py#L170.
>
> Invalid:
> This is your personal taste, but simply the required control flow in these
> methods.

Disagreed, it can be done so much better.

Do you have an example of how you would replace the control flow _in your own better way_ ? 

>
> ===
>
> In valid review would have at least touched on the aspects of testing! Units
> tests, coverage, integration tests, ..
>
> You would have touched aspects like outside contributions, number of
> open/closed issues, time to resolution, etc etc
>
> I could go on. I guess any seasoned Python developer would expect _much_
> more from a text that claims to "do a thorough code review" of a 40k LOC
> code base.

Given the time limitations, and the fact I had already made a
conclusion decision to say no, it did not make any sense to dive
deeper.

If you have time limitations, don't do it. When I've read your draft, I've felt you're trying to expose a completely outdated state of a project which moves very fastly.

If you want to give real and serious review to your readers, you *need* to dive deeper, and explore everything. It is like you're judging a book by its cover. 

>
> ==
>
> Anyway. Please all: let's move on and stop wasting time.

The fact that you see discussing ways to improve code quality as
"wasting time" speak volumes in itself.

I think that it is because the way you're discussing it make me think (and make us think) that you're trying to conclude that the WAMP subprotocol as a whole has no value.

Naturally, as a user (and I've dived deep into Crossbar's code), I don't think that you're opinion is really relevant as it is mostly outdated and personal.

Then, I agree with Tobias that it is a waste of time. Discussing ways to improve code quality may be done in a lot of better ways than writing an article to say "I would not use WAMP to write application". 

Kind regards,

Raito Bezarius. 

[Cal Leeming]

Thank you for your feedback, although I disagree with some of your
comments, your input is appreciated.

In ref to crossbar source, I will re-evaluate that part of the review
as much of it has changed since Dec 2014. This also leads nicely onto
the lack of "maturity/stability" argument, which I'll also provide
more thoughts on.

As a final note, it seems the primary focus is on rejecting my final
decision, which is subjective, followed by dismissing very clear and
valid technical arguments on code quality. There again, this is an
understandable response given that typically my writing style doesn't
invoke positive feedback from maintainers unless it's speaking in
their favour. Never the less, despite the clear negative emotional
response from the maintainers, I'm hoping this will drive better code
quality in future, or at the very least encourage others to review the
code and speak up.

Thank you all again for taking the time to respond.

Cal

> https://groups.google.com/d/msgid/autobahnws/afc4e4bd-00e8-4a26-b28a-b6ea0bdbff5f%40googlegroups.com.

[Tobias Oberstein]

> As a final note, it seems the primary focus is on rejecting my final
> decision, which is subjective, followed by dismissing very clear and
> valid technical arguments on code quality. There again, this is an

No. See my other mail: the code quality issues you raise are factually
incorrect or invalid.

> understandable response given that typically my writing style doesn't
> invoke positive feedback from maintainers unless it's speaking in
> their favour. Never the less, despite the clear negative emotional
> response from the maintainers, I'm hoping this will drive better code
> quality in future, or at the very least encourage others to review the
> code and speak up.

Well, I am sorry to disappoint you here, but while we take code quality
very serious, your text doesn't help.

If you really are interested in improving the code quality, filing
conrete issues or actual code PRs would work much better.

Cheers,
/Tobias