[Development] [API Review Request] QWebsockets

[Matt Broadstone]

On Sun, Aug 25, 2013 at 3:49 PM, Kurt Pattyn <pattyn.kurt at gmail.com> wrote:

> Hi,
>
> I am about to move the QWebSockets project (see:
> http://github.com/KurtPattyn/QWebSockets) to the playground.
> Could you please review the API?
>
>
Just some quick stylistic observations:

* you don't usually make a class_p.cpp (implementation) file, you only need
to do that for the header so that you don't export those symbols

* looks like you are using tabs all over the place

* I think it's part of the style guide to use the Q_D and Q_Q macros to
access your classes d and q pointers, respectively.

* imho a lot of this code could be squashed together so as to reduce the
number of files:
   - qwebsocketprotocol can just go into the "global" header, since it just
defines some enums
   - classes like "DataProcessor" and "HandshakeResponse" could probably
just move to the implementation of QWebSocket

* unit tests are woefully incomplete, also you seem to be running qtestlib
manually.. I would suggest taking a look at Qts test dir or something like
it for inspiration here (many folders with a set of tests in them). This
part is probably going to be most difficult, as you'll be writing tests
after you've seemingly completed the implementation.

Cheers,
Matt

Notes:
>
>    1. The public API is QWebSocket and QWebSocketServer; all other
>    classes are internal.
>    The APIs of both classes were modelled after QAbstractSocket and
>    QTcpServer. This should it make easier to use the API.
>    2. I have also tried to make the directory structure compliant with
>    the Qt guidelines; so, if there are any mistakes, it would be good to know
>    as well
>    3. I created a qdocconfig file, but it misses links to the Qt library
>    classes
>    4. The sync.profile has been created by looking at the QtSerialPort
>    project, but I am not sure if that is correct as well
>    5. Unittests still need work to be up to the Qt standards
>
>
> Thanks for your time.
>
> _______________________________________________
> Development mailing list
> Development at qt-project.org
> http://lists.qt-project.org/mailman/listinfo/development
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.qt-project.org/pipermail/development/attachments/20130825/7496dac7/attachment.html>