Hi Atul, you’ve done quite some work!

I agree that isValid() should be removed! If we use the new version of parse() that gets called from the abstract PacketFactory, then we can even get rid of the isValid private member.

By null object pattern you refer to the use of BadPacket I guess. Am I right?

I see you removed the call to parse() from the raw data ctors. I thought it would be a good idea to get rid of these ctors altogether (I plan to do this for BitTorrent packets but will also need to modify the unit tests). Using the new version of parse() would be equivalent.

I liked the refactoring of parse() which now calls processHeader() and processPayload(). I would suggest that instead of QDataStream we use BinaryReader and BinaryWriter. Actually derived and extended classes containing also Gnutella specific read and write functions.

I actually really like the idea of processHeader() and processPayload()! They could be matched to readHeader (const QByteArray &) and readPayload (const QByteArray &) in the new Packet system and they can call the new virtual functions of Gnutella Packet readHeader (BinaryReader &) and readPayload (BinaryReader &). I like the idea so much, that plan to do exactly this with BitTorrent packets, I think that will be some good refactoring! I didn’t like the Q_ASSERT ((quint32) rawPayload.size() == payloadLength()); in each concrete BitTorrent packet. This new approach gets us rid of it!

I don’t get it why you made rawHeader() and rawPayload() virtual. They were designed to do some static checks and call prepareWriteHeader() / writeHader() and prepareWritePayload() / writePayload() only if header/payload needs to be rebuilt.

hosts.dat and Makefile are also included in the patch. If you use bzr diff > mychanges.diff it will create a diff of only the changes to files that are in the bzr branch. host.dat will be also included but I always do bzr revert hosts.dat if it has changed and want to create a patch.

Maybe you could copy your modified packet files to Protocols/Gnutella/Packets and try to use the new abstract base class from Protocols/Generics? Doing so would be a great way to verify how good the new base class is and will help to improve it.

Best regards,

Peter

atul wrote:Hi all

as this needs some more polishing.. the major change is removal of isValid and putting in the null object pattern…

how does it look ….

–cheerio atul