Hi Peter

Yes I agree with you… What I meant was there should be only one place where the switch happens ( and the create packet should return a Packet* but bound to right runtime packet… )

Everything should be done in this method ( that  way it becomes cohesive ).
You are right in that we should have only a single bad packet ( as it is just another PACKET )

– cheerio

atul

Peter Dimov wrote:

Hi Atul,

You mean that the implementation of the pure virtual createPacket() should be allowed to return e.g. Protocols::Gnutella::Packets::BadPacket instead of Protocols::Generics::BadPacket? What would be different between the bad packets of different protocols? I thought they would only store the bad raw data.

I think that with the new version of parse() we won’t need the “raw data” ctor anymore and parse would also need to assign raw* to the corresponding members. I had forgotten to do the assignment. Otherwise I agree with your parse() refactoring. I just fixed it locally and it will be included in the next patch.

Regards,

Peter

atul wrote:Dear all

I think the createPacket function itself should call the parse function ( so it can completely determine the kind of packet to return )

So only createPacket would remain..

A little refactoring ( I think that applies to current code too )
bool Packet::parse (const QByteArray &rawHeader,
const QByteArray &rawPayload,
const QByteArray &rawTrailer)
{
Q_SD (Data);
bool isValid = true;

isValid = isValid && readHeader (d->rawHeader);
isValid = isValid && readPayload (d->rawPayload);
isValid = isValid && readTrailer (d->rawTrailer);

d->isValid = isValid;
return isValid;
}
Could be reduced to
bool Packet::parse (const QByteArray &rawHeader,
const QByteArray &rawPayload,
const QByteArray &rawTrailer)
{
Q_SD (Data);
d->isValid = readHeader (d->rawHeader) &&
readPayload (d->rawPayload) && readTrailer (d->rawTrailer);

return d->isValid;

}
Or am I missing something? ;-)

— cheers atul