Dear Peter

My replies in bold

Peter Dimov wrote:

Hi Atul, you’ve done quite some work!

Thanks mate
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.

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

Yes … This is largely based on the example given in Refactoring by Martin Fowler…
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.

Yes removing ctors would be a nice refactoring.
I don’t get it why you made rawHeader() and rawPayload() virtual.

The idea is Packet is an interface ( somewhat like a java interface ). Now suppose somebody called a readPayload on a BadPacket… what should be the behaviour? Should it throw an exception?

I am not so sure if it should throw as exceptions are used for really exceptional events… They are not used for errors… AFAIK ;-) 
Note that we have a null object in the first place so that isValid checks are not done anymore… throwing exceptions ( and handling them ) would bring it back ( in another garb ;-)

So as of now I do nothing… Null Object  should blend with other objects and not stand out ( AFAIK that is ;-)

They were designed to do some static checks and call prepareWriteHeader() / writeHader() and prepareWritePayload() / writePayload() only if header/payload needs to be rebuilt.

Could you elaborate a bit on this ;-)

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.

Yes I think bzr needs python 4.0 I think so I will set up and start using it ( using version control is very convenient )
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.

Sure ..

— cheerio atul
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