Hi Atul,

You are again making a good point, this time about how and who generates this PacketInfo struct from a Packet object. Your way of thinking complements with what I was thinking about in respect to generalizing the packet abstraction (more details about that in a separate post soon).

PacketInfo is created and filled by PacketModel, the data of which is displayed by PacketDumpTreeView. The classes are not in use now because I disabled them after some major refactoring in the past. Anyway, we should enable them some time again and we should refactor them in a way that would enable dumping of new packet types easily (for example packets of BitTorrent peer connections).

Now, following your proposal for a factory method, the question is whether we add a pure virtual function virtual PacketInfo packetInfo() const = 0; to the base class of all binary packets, from which Gnutella::Packets::Packet and BitTorrent::Packets::Packet will be derived? I like the idea! What I’m still not sure about is what exactly will be the mechanism for dumping the packets and what information can be generalized. In other words, which object adds them to the PacketModel (maybe choose a different name, or promote this to a service -> Services::Monitoring::PacketDump)? We should be able to do filtering based on protocol, packet type, etc.

Do you have any further comments or ideas in this respect?

Regards,

Peter

atul wrote:

My replies in bold …

Peter Dimov wrote:

Hi Atul,

I was actually asking myself that exact question when I was designing the Packet class. Getters are necessary in any case to extract the data. If we had a bigger number of overloaded ctors we wouldn’t have needed the setters. I felt however that would lead to an excessive number of ctor, which would make the class interface and implementation more complex and less flexible.

What works, AFAIK, is the design which evolves. As you rightly put it, there are many options… And the more you keep refactoring ( tinkering?;-), the right design comes out…( right design is a bit subjective term ;-)

this is what XP advocates.
Generally, the approach I took with Packet and its derived classes was to keep it as simple and generic as possible by having it know as little as possible about the different context where it can be used.

Yes I love that… It is a neat thing to have ;-)
My personal preference is to build up complexity by stepping on simpler and more generic concepts, instead of stuffing a hell lot of things in a presumably “clever” object.

I have had similar experience… Nowadays, I would settle down with a simple design that works… and then refactor and weigh and look at what is more elegant/natural fit…

I am looking into

bool PacketModel::insertPacket (const Packet *packet, Direction direction)

and here packetInfo should be generated by Packet… This goes by the name of “FACTORY METHOD” pattern…
Incidentally, I was skimming thru “C++ Common Knowledge” and one item gives a very similar example… ;-)

Now I think that is a right way to extend Packet. And follows that above idiom so I feel I am going in the right direction…

atul wrote:Dear all,

This is one of the two design points ( the other is about Null Objects ), that were coming nagging at my mind ;-)

I think giving set/get harms encapsulation ;-) Some code could erroneously set only one of them and take the object in a wrong state ( One way to look at objects is that they model states ;-)

Instead, I think we should look at what the outside code is doing with these get / set and

remove these set / get

push that operation to the object ( it could internally use setters / getters and always maintain a coherent state )

Some Get operations are ok ( like name() ) but things like setTtl()/ttl() maybe are not, IMHO ;-)
What do you all think?

—- cheers atul