Hi Atul,

Thanks for the comments! Let me see whether I got your point right. You already mentioned null objects in another context. If I’m not wrong you mean objects like QByteArray::Null or QHostAddress::Null. A default constructed object would be equal to the Null object. So more or less, the Null objects are valid objects and one can call e.g. toString() on them.

I have difficulty however to think of how to create a Null object for a polymorphic type like Packet. My concern is, whether there should be only a Packet::Null, or also Ping::Null and Pong::Null. We might get in trouble doing compares of Null objects in the latter case.

I think that currently isValid() is required because we need to distinguish between packets which were successfully parsed and ones that couldn’t be. Maybe we need to rename isValid() to something that makes more sense? We could set a packet that cannot be parsed to Null instead, but then we would not have access to any further information for this specific packet object. For example, we wouldn’t be able to dump Null packets because they have empty header and payload. Also, a null packet having zero-length header and payload is actually not a valid packet when written on the wire, so I’m not sure it would be very suitable to apply this Null object concept here. Could you provide some arguments?

Regards,

Peter

atul wrote:

Wow mate I had a quick look and that looks pretty neat…

I just have one suggestion however… How about using NULL OBJECT so that all the

if( !isValid() )

checks need not be done…

The idea is there is only one invalid packet object ( a singleton ) that returns an empty QByteArray() when asked to execute rawheader() etc… )

As the NULL OBJECT obeys the same interface as packet it will always be correct ( i.e. there is no need for the checks and the checks are never missed )

I will submit a patch after a week but this is on my mind for long so what do you all think?

See

http://www.refactoring.com/catalog/introduceNullObject.html

for example…..

— cheers atul

Peter Dimov wrote:

Hello everybody,

I finally publish some preliminary version of the Packet base class, which should be used as a base for Gnutella, BitTorrent and other future protocol packet implementations. It should make it easier to add support for a new protocol based on exchange of binary messages. In this post I have presented some of the questions I’ve been asking myself regarding the design of Protocols::Generics::Packet. I hope that I will get some feedback from at least some of you. Exchanging some ideas would definitely contribute to achieving a better solution.

I based my current work on the precious Gnutella Packet implementation. Some documentation may thus not be adequate. The function comments will be moved to the cpp file, so that lines not exceed a length of 80 characters.

I’m wondering whether the trailer functions should be commented out or removed until we really have to implement a protocol that uses some trailer data. Otherwise I would be adding something that never gets used, or is inadequate to a later arising need.

Just as an idea, we might at a later point add some functionality to sign packets.

Are the pure virtual functions name() and protocol() required? They might be of help for debugging purposes or for classes like PacketDump or PacketMonitor. I’ve added an accept() function for the visitor patter which Atul applied on the Gnutella Packet class. When we have the visitor pattern we might create a concrete visitor that returns the name or protocol of a packet object.

I also have the idea to declare structs for Header, Payload and Trailer, from which the derived Packet classes should derive their own structs. The point is to have the Packet base class to take care of reference counting (there are some examples for that in the source of Qt). What do you think of that?

It would maybe also make sense to derive the GGEP extensions base class from the generic Packet.

I still haven’t figured out how to ensure some invariants like length of raw payload data be the same as the value in the packet header. Maybe the derived classes should care about this. I guess I’ll have to do first experiments with BitTorrent and get some feedback.

Regards,

Peter