Hi Peter,

I really appreciated the idea with problem description when I was reading the docs. It’s also good that there is a small example how the inheritance will be done!

I’ve got one question about naming and placing. In the example the ConcreteSession is derived from two classes (GenericSession, SessionStatus). 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”?

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

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

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? 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?

–

Todo: Another approach (that would also be easier to test and thus probably better!) would be to have a GenericSession object as a member and just delegate the Session functions to GenericSession. Update the example and description to it eventually!

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).

–

The GenericSession interface seems clear to me at the first sight, lets take a better look.

Todo: The underscore coding convention! Having underscore suffix in member variable names will make the implementation of the class ugly. Having the underscore suffix in the parameter names would make the docs ugly. If the “param” names are different than the actual parameter names Doxygen will print a warning. What to do!?

That remainds me that if you want in Python declare some variable/function private you have to put two undersoceres (__privateFunction()) before it - as far as I know there is no other way, so you can easily imagine the code (brr)… 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.

Todo: In general, should we document the status notifications that possibly get sent, or should the user have a look at the test too? Should we add see also links to relevant tests and or scenarios? That does make sense especiall if we say that our scenarios are a kind of a specification.

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.


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.

–

AbstractValue/Data
It’s good that there are written requirements for the template parameter T in the AbstractValue class. The idea is also great.

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.

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.

Todo: Automatic casting from AbstractValue <DerivedClass> to AbstractValue <BaseClass>? What about the other way around?

Hmm, that would be a “nut” :). Declaring a cast operator AbstractValue <T> () 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 <typename Base>
AbstractValue <Base> cast ()
{
auto_ptr <Base> base (object_.copy()); // I think that this would cause a compilation error if the conversion is invalid
return AbstractValue <Base> (*base.get());
}


–

Ok, lets take a look on the GenericSession tests.


class DataChecker
...
typedef bool (Function) (Data &);


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!

\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.

–

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!.

Have a nice day,

Petr