Hi Atul,

Well, Packet is not really an interface although it is an abstract class. It provides implementation for some very basic packet functionality and declares some pure virtual functions, which the derived classes should implement. I think that is known as the template method pattern. Since all our template methods, including readPayload(), are protected functions, it is not possible for a client to call directly parse() or readPayload(), so we shouldn’t have any concerns about anyone calling BadPacket::readPayload(). We can put a false assertion in the implementation, so that we directly will know if bad code allows the function to be called.

I’m not an exceptions expert and I prefer not to use something I don’t understand… but that means I better get a book and read more… but I already have pretty much on the read queue ;-P. I’ll be pretty happy to hear recommendations from an expert where it would make sense to use exceptions in Calitko!

Generics::Packet::rawPayload() is related to Generics::Packet::writePayload() in the following way:

//! Returns the ready for transmission raw payload bytes.
/*!
	The function returns a cached QByteArray containing the raw payload bytes,
	unless the payload was marked as modified by calling invalidatePayload().
	In this case, the payload is rebuilt by calling writePayload() and cached
	for future use.

	Caching of the packet's raw data improves performance especially when the
	same packet is sent to more than one peer. In this case, due to reference
	counting, the raw data is generated just once and is also stored just once,
	although the packets are copied in multiple connections.

	sa rawHeader(), rawTrailer()
*/QByteArray Packet::rawPayload() const
{
    Q_SD (const Data);
    if (d->isValid && d->rewritePayload) {
        d->rawPayload = writePayload();
        d->rewritePayload = false;
    }
    return d->rawPayload;
}

d->isValid will be deleted soon of course. Similarly, parse() is related to readPayload().

Yes, Bazaar requires Python 2.4 and it is really great! Still there is no good support for the IDEs but using it from the command line is quite intuitive.

Best regards,

Peter

atul wrote: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