Hi Petr,

Thanks a ton for the detailed comments!

I’m not sure if I understand it correctly (and please correct me if I don’t), but the SessionStatus would be some other class that is derived from the original SessionStatus class or the original class itself? Which of these examples is (more) “correct”?

I think one could chose each of the two possibilities that you suggested. It would depend on the situation I guess. I was thinking about this case though:

class ConcreteSession : public Protocols::Generics::GenericSession, public Protocols::Generics::SessionStatus {...};

My original idea was that ConcreteSession could for example implement SessionStatus::sessionReadData (const Data &) and then possible send different notification over a more specified interface. For example, BitTorrent TransferSession would provide TransferSessionStatus having e.g. choked(), unchoked(), etc. sessionReadData() would be implemented to check the runtime type of the Data object and if it is a Choke packet, choke() will be called.

Generally, one should name all session status classes SessionStatus and place them into the same source (header) file or name them differently (for example ConcreteSessionStatus) and place them into another source file or some mix of these?

I do not have a definite opinion about that actually. Interface and InterfaceStatus are separate classes, so even though they are tightly related it might still be good to put each in its own module. Not all classes would need both interfaces. For example, a DataSerializer implementation would need to know Transport but would not need to know TransportSession.

Initially I would put all stuff in one header file and would eventually move them to their own modules after the names become “stable”. See for example GenericSession - originally it was called PacketSession. There was no Session interface at first. It was much easier for me to put all interfaces (possibly duplicating some) in the same header where the concrete class I’m testing (and developing at the same time). Keeping everything in one place gave me a better overview and a kind of “separated” work environment within Calitko. Now that GenericSession is being finalized, I’m starting to move the individual interfaces to their own modules (a revision is underway). That’s what TDD taught me and it seems like a good approach!

Everything depends on whether the SessionStatus class would be considered as “final” or it will be allowed to derive other status classes from it. If I took a look on my current TrackerRequestSession (lets forget that this case is different than the GenericSession case), should I name the TrackerRequestSessionStatus class just SessionStatus or keep it?

I’ve asked myself the same question - does it make sense to declare interfaces that derive and extend one or more others? I’ll have to check whether Java and C# allow to derive one interface from another but I can think of a situation where this could make sense. Transport could derive from both Buffer and from Connection.

I can’t imagine that now, because I think that inheritance is a better (clearer) solution than composition in this case, but maybe there is something I don’t see :) (if I’m right, we’ve used this approach in our PacketBase/Packet classes, so maybe it will work).

I’m now quite certain yet. PacketBase is a different case though - it has more data than behavior. With ConcreteSession that derives from GenericSession the problem is how to test it. Let’s take the BitTorrent TransferSession for example. It could provide a function called choke() that would simpy call send (Choke());. How do we test that if send is a function implemented in the base class - no way to mock it. I’ll soon be trying that out, so I’ll be able to give more details then.

Anyway, I’d suggest to use underscored names only in private data members, hence parameter names would be “clear”. Another approach would be to name parameters differently, but sometimes it’s at the expense of readability, so it’s bad. Third and last approach could be to use PrivateData struct member, so you can distinguish between two names by their “namespace” (transport vs d->transport) - but this approach is more useable for value classes like Torrent or TrackerRequest and here it would lost its main meaning. I’d personally stick to the first approach.

I like Qt’s d->var style a lot but I agree it is great for Value Objects but an unnecessary overhead for Reference Objects. Readability of docs and public parts should have priority so I’d agree we’d better stick to the first approach - seems like the worst of all evil.

The idea with adding links to relevant testing scenarios makes sense to me. Since there is usually showed the way how to work with the class and do stuff, it’s worth from my point of view to document it. As for the status notifications - I don’t know. If that’s relevant, we should document it too, but I don’t have enough experience with it yet so I can’t tell.

OK, I’ll experiment with linking to test cases and scenarios - the status notifications could be seen from there.

class DataSerializer
{
...
virtual bool write (const Data &) = 0;
virtual void tryWriteBufferedData() = 0; // \todo remove?
};

What is the difference between these two member functions (i.e. what tryWriteBufferedData() does)? I think that whether to keep it or not it depends on the write() function behavior. If it can fail (invalid object or something), then maybe we can keep it, but for that the write() function already returns bool. So maybe we should remove it.

I already removed it. The point was that the serializer could buffer parts of the last Data that couldn’t be written (Transport buffer got full) but then I realized that the GenericSession implementation would become uglier if tryWriteBufferedData() were used. Especially, how would GenericSession know when the data buffered in DataSerializer is already written to Transport? After close() has been called, GenericSession should only disconnect the Transport after all queued data has been written! I also thought that an interface with just read() and write() functions is much clearer and easier to use.

The reason I had tryWriteBufferedData() in the first place is because PacketBuffer has it and it has it because of its specific implementation (a good sign it does not belong to an interface ;-) ) - a Packet consists of three parts (header, payload and trailer). It is possible that the Transport writes the header but cannot write the payload. Then the payload and the trailer must be buffered. An easier but less efficient solution would be to concatenate the three parts and write them as a whole. Yet another solution would be to add a canWrite (int) function which could be called to verify whether header + payload + trailer could be written in three write() calls. canWrite() does not follow the “Tell, Don’t Ask” principle but I’d consider this principle as a guideline rather than an actual rule ;-).

Todo: Move to its own module (probably somewhere in Utils?)

I’d agree - we should think where to put classes like this or Uri (maybe after some another class is made it will become clear and obvious). So lets place them in Utils for now.

OK!


Todo: Allow to pass the NullValue type as template parameter?

I don’t think that it makes much sense to allow it. I haven’t see it anywhere - is this (passing some type that depends on the type parameter as a type parameter) even possible in C++? :) Maybe you ment something different by that.

I mean PacketBase and BadPacket as an example. The problem is that we need a default ctor for AbstractValue but this default ctor needs an object derived from T. With the current implementation Packet() would be different from Packet (BadPacket()) even though BadPacket() is the Null Packet object. Maybe we shouldn’t really care about that until it actually becomes an issue!

Hmm, that would be a “nut” :). Declaring a cast operator AbstractValue () would work only for casting to a class specified as a template type parameter. I’m just thinking, wouldn’t it be possible to create something (a member function) like this?

template
AbstractValue cast ()
{
auto_ptr base (object_.copy()); // I think that this would cause a compilation error if the conversion is invalid
return AbstractValue (*base.get());
}


Yes, that snippet would do the job! Casting Data to Packet would need some kind of custom dynamic cast though…

and this line of code:
.willCall (serializer->read (checker (DataChecker (data1))))

Maybe you could write some docs to it, because I’m quite confused what is actually going on there :). But I have a feeling that it’s related to CalitkoMocks somehow - just guessing that the data are read into the checker and then they’re compared to the data1… Except these things the tests are really clear to me!

I’ll do write some docs! Yes, it’s CalitkoMocks related - it defines a custom expectations checker. You need one when the mocked function is supposed to modify a parameter (like in the case with bool read (Data &)).

\todo connected() must not be called as it will assert. Find a way to test that! Maybe assert throwing and exception will do.

I was thinking about the same thing some time ago. The “exception throwing assert” seems to me as the only possible (usable?) solution. Q_ASSERT exists the program if the condition is false, so the only way how to test it would be some kind of “return during exit program execution hack” (uhm…), if any.

Some custom assert macros that throw an AssertionException would do the job.

I’ve been thinking about testing the preconditions but I think that strictly speaking, Design By Contract does not say that the implementation is even required to assert when a precondition is violated. Some preconditions are hard to check for example. The idea of Design By Contract is that the implementation must fulfill the postcondition as long as the user fulfills the precondition. If the precondition is violated the behavior of the implementation is undefined (there are some “undefined behavior” cases in the std-c++ too). So I’m not really sure whether we should test preconditions even though I’d like to ;-). I guess that question would stay open for a while…

I’ve written quite a lot of things, but I hope it could be useful for you to check them out :). I was writing them during my docs/code reading - nearly everything that comes to my mind. The overall feeling is great, though!.

Petr, again, thanks a lot for the comments! They are indeed really helpful! It’s always great to hear someone else’s opinion and get a different perspective! I’ve added a few more items to my TODO list :-)

Have a nice day too!

Peter