Hi Atul,

I love the idea! Never thought of that!

You’re quite right that using a visitor will get us rid of the switch statements and the compiler would be able to give us some further hints.

Your patch shows quite nicely a further benefit of using *Private classes. The pointer to the private data can simply be passed as an argument to the visitor’s ctor. If we didn’t have a private class, then a pointer to the “real” object had to be passed instead, which would have very probably required a friend declaration of the visitor class in the header. As you have nicely done it, the visitor class can be completely declared locally to the source file that uses it and this is perfect implementation hiding!

Especially for PacketProcessor it would make sense to have a visitor (maybe better called xPacketHandler, or xPacketDispatcher, etc.) for each of the functions packetReceived(), notifyHandlers(), routePacket() and sendPacket().

Before going further with applying the visitor patter I’d like to reflect on how this proposal could be applied to a more abstract Packet class (the one that will soon come in Protocols::Generics). We could declare an accept() function in this base class which takes a visitor of type Generics::PacketVisitor. The base classes of Gnutella and BitTorrent will implement this function and define an overload taking e.g. Gnutella::PacketVisitor. The generic visitor will only know about the base classes and the specific visitor will know about the concrete classes derived from the bases.

If such approach is used, then instead of declaring a pure virtual function packetInfo(), we can create a visitor that extracts the corresponding data from each packet type! A solution I much better prefer than the one with a pure virtual function. The reason is that with the visitor the packet classes can stay as simple as possible and the “make them clever” functionality can be implemented in separate visitor classes.

There is one case for which the visitor patter will not be good, however. That is if we want to make it possible to extend Calitko via plugins at a later time. The problem with a visitor is that it needs to know all subclasses of a class at link time. With plugins, which could come from third parties, we cannot assure that.

A solution might be to somehow use the Qt metaobject system to have “dynamic” visitors. I haven’t looked into the details yet, that’s just an idea. Another disadvantage of visitors is that we will have a big number of classes. It would be great if we could instantiate from a “dynamic” visitor class and initialize it with the function that should be called for each of the concrete type. So, maybe something like QSignalMapper (which maps from the signal sender address to an ID) but one that should map the type of the sent object to a slot.

Another solution would be to simply drop the visitor for the generic packet, but use it for the base classes of the concrete protocols. I can imagine protocol implementation consisting of multiple plugins. For Gnutella, for example, that would be PacketProcessing and Transfers, which are largely independent. Problematic will be if we want to split PacketProcessing into a number of plugins each of which introduces different packets. I don’t think something like that would make much sense though.

Enough for now from my side, as the post already got too long. I’m looking forward to getting your comments, Atul!

I hope that also some other of you guys would like to take part on the discussion and give some interesting ideas!

Regards,

Peter

atul wrote:Dear all

This was on my mind for a long time ( removing packet.payloadDescriptor() and switch/case statement … )

OO recommends removing this … So what is the solution?

This is where the Visitor pattern ( GOF ) fits very nicely…

So I put in the Visitor framework and made

PacketProcessor::packetReceived

a visitor….

Here is the patch…

Let me know what you all think…

( One nice fallout is if we add any new packet the compiler will help us make sure all the relevant code gets updated ;-)

— cheers atul