Dear Peter
I was not able to extract the patch on the source branch … However I could read it fine…
You have a great idea there and yes that is what our factory should do….
However, do we need to write the factory method a functor? I mean conventionally we use create/clone/get with factories… I would call it as getPacket ( not use the operator()() overload )
( Especially as it need not be called the same way as a global function here )

Also compare boost::scoped_ptr with auto_ptr … Next post I will post a book excerpt to illustrate the difference…. Do we need the ownership transfer semantics here?
What says everybody?
— cheers atul

Peter Dimov wrote:

Hi Atul,

I’m glad you’re back with useful comments! I finally understood your point and attached is my interpretation of your idea. I still haven’t removed isValid but if all agree on the presented approach, I will do so with the next patch (I’ll need to do some modifications in the BitTorrent packets as well).

I thought it would be nice to make the packet factory a function object. That would be especially useful if we want to have a generic PacketReader class read any kind of binary packets as long as the necessary helpers are provided. These would be a functor to extract the raw data from the underlying Connection and another one to create the Packet object from the raw data.

Here are the modifications in Packet. I changed parse() to return bool and take the raw bytes as arguments. This would mean we don’t need the constructors taking raw bytes as parameters. If the new parse() returns false, that means that the raw data could not be parsed and a new object of type BadPacket is created from this raw data. If we return a singleton object, then we would not be able to dump the raw data and won’t be able to analyze whether the packet was really malformed or our implementation has a bug.

The patch also contains some minor modifications in fulfillment of the coding conventions. Most important are the new files BadPacket.{h|cpp} and PacketFactory.{h|cpp}.

What do you think of this solution, Atul? The others of you guys? Further suggestions?

Best regards,

Peter

P.S. Either check the patch, or pull my public bzr branch http://bzr.calitko.org/developers/peter/calitko-BitTorrent

atul wrote:

Hey Peter and everybody

Sorry for the late reply…

My idea was like this..

Packet factory would return a class which will be a wrapper for existing Packet… It will overload operator->() so delegates all the calls to the internal packet pointer…

Now here it could make a check for the validity of the packet and if the packet is not valid return a singleton packet pointer ( which is BadPacket ;-)

This way the check will never be missed and is guaranteed by design ;-)

—- cheers atul