[Development] [API Review Request] QWebsockets

[Kurt Pattyn]

Ok, tabs has been converted into 4 spaces.

Regarding the *_p.cpp files: I just did this to keep the file sizes at a reasonable size; if it is a style requirement, I can merge those files.
The same holds for the other files (DataProcessor, …). If I merge them with QWebSocket.cpp, then that file will be really big; isn't that a problem then?

For the Q_D and Q_Q macros, I will look that up (unless someone has a reference to the rule); I found Qt code that was just using d_ptr and q_ptr, or even d_func().
What's the general rule?

Yeah, unit tests are incomplete; I am aware of that. Currently, the only tests I have, is against the Autobahn Testsuite (both client and server), which I start manually. That suite is testing all normal, border and error conditions (based on RFC 6455). Memory leaks have been tested with Instruments on Mac OSX. For the rest, the code is making use of QTcpSocket and QTcpServer. I'll expand the unit tests in the coming days. I suppose I cannot run Autobahn on the Qt infrastructure?

Best regards,

Kurt

On 25 Aug 2013, at 22:29, Matt Broadstone <mbroadst at gmail.com> wrote:

> 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:
> 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.
> 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
> I created a qdocconfig file, but it misses links to the Qt library classes
> The sync.profile has been created by looking at the QtSerialPort project, but I am not sure if that is correct as well
> 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/ec548798/attachment.html>