Hi Peter,

I tried to understand the test cases but unfortenately I was not able to start the program in the debugger.

I do not have really much expierence in code reviews but here is what I have seen.

The first thing was the header guard it started with two underscores. I have learned that one or two underscores a resevered for the compiler builder (C++ specification):

17.4.3.1.2 Global names [lib.global.names]

1 Certain sets of names and function signatures are always reserved to the implementation:

— Each name that contains a double underscore (_ _) or begins with an underscore followed by an uppercase
letter (2.11) is reserved to the implementation for any use.

— Each name that begins with an underscore is reserved to the implementation for use as a name in the
global namespace.)

Then there are the two macros (FORWARD_DECLARE and CALITKO_TESTABLE). I do not know excatly the project policy but this two marcos made the headerfiles not reusable. Because if another progammer what this class, s/he must remove this macro. On the other hand in my opinion the production code should never know if it is running under test or in the real project.

For readability I would found it better if interface would start with an bit letter I (ISocketBuffer), classes with a C (CTypSocketBuffer) and abstract classes with an AC. And I would prefer is member variable starts with m_ and maybe the hungarian notation (m_pReadBuffer).

The interface SocketBuffer was ok. The implementation of the destructor was in the header file - so the question is why is there an implementation file?

In the header file of TCPSocketBuffer I made normally the derived methods virtual. (virtual QByteArray read(int count)); I think is has something to do with the method hinding - but I can not remember for the reason.

On the first view I found it a little bit strange that so many private methods. This means you can them test only indirect.

In the constructor of TcpSocketBuffer I would prefer the initialization list. (see Scott Meyer, Effective C++).  I found out that the setReadBufferSize and setWriteBufferSize are only invoked in the constructor and do more or less the same stuff as before (initialize the member variables). I think that the Q_Assert is wrong it must be greater 0. But unfortenately this information will not give to the client. To create this object is only possible with readBufferSize and writeBufferSize greater one. So if somebody creates the object with TCPSocketBuffer(-1, -1) there should be thrown an exeption if it is done in the constructor. Otherwise it should give another method which returns a value and should be invoked after the creation - like bool createReadBuffer(int size);

Then I did not understand why the docuemation is in the implemetation file and not in the header file.

The method canRead, canWrite, and hasRead, nextWriteChunk etc can be const.

The other question which arise was why are the member variables readBuffer and writeBuffer are not a QByteArray? Or the other question why are here Qt lib is used? If the Calitko Team will decide to change the GUI to another you will have a dependency in the TcpSocketBuffer.

Then I have to say that I lost a little bit my interest because I was not able to write some test and see what the class is doing.

So I want to write test cases with

TcpSocketBuffer tsp(100, 100);

tsp.read(-1);

tsp.read(0);

tsp.read(100);

tsp.read(99);

tsp.read(101);

So I did not understand it really good.

But then I found something really strange. The function tryToReadFrom and tryToWriteFrom have a dependency to the socket. But the socket are network stuff. And  I believe that the direction is wrong the owner of the socket should know the TCPSocketBuffer and not vice versa. I have not looked who is the client of this method. But I think here will be the Single-Responsibility-Principle broken. (http://en.wikipedia.org/wiki/Single_responsibility_principle)

I hope you can follow my thought. First I was really motivated but then I was recognizing that I can not use the linux GNU stuff. So the things get complicated. Maybe I should switch to Visual Studio 2005.

Regards,

Markus