For the third point — CreatePacket takes PacketData

Certainly. I think you fell into a trap in making the factory take the ID of the kind of packet to create. As mentioned in First Point, you used that instead of creating normally when you already knew the type you wanted, on the sending side.

So yes, have a static factory include the logic to figure out what kind of packet it is, hiding that detail from the users of the class.

Then, since the factory has the data, why not have it do the deserialize too? You are already dispatching to a CreateMBXPacketCb function that’s written separately for each class. So pass the data through to that!

auto msg = Factory::Create(packet_data);

figures out the type from the data, and then calls the proper

unique_ptr<MBXGenericPacket> WhateverClass::CreatePacket(PacketData);

the only difference from what you have being that CreatePacket takes a parameter now.

Within the functions:

//static 
unique_ptr<MBXGenericPacket> WhateverClass::CreatePacket(PacketData data)
{
auto self= make_unique<WhateverClass>();
self->Deserialize(data);
return self;
}

The forth point too!

With the above design, nobody needs to be exposed to the type ID other than the factory function itself and each class which knows to initialize itself with the proper value when creating an empty message for sending.

##Advanced

From my generic CreatePacket above, you can see that each one is exactly the same except for the name of its own class.

This suggests that you make it Generic for real. You only need one template function.

The only reason I say this is advanced is because of how you’re dispatching on the type to registered functions. You might scratch your head for a moment to figure out how to take the address of a specific instantiation. Then, you wonder why you need all those lines to populate the map when they are all the same except for the type name...

For the third point — CreatePacket takes PacketData

Certainly. I think you fell into a trap in making the factory take the ID of the kind of packet to create. As mentioned in First Point, you used that instead of creating normally when you already knew the type you wanted, on the sending side.

So yes, have a static factory include the logic to figure out what kind of packet it is, hiding that detail from the users of the class.

Then, since the factory has the data, why not have it do the deserialize too? You are already dispatching to a CreateMBXPacketCb function that’s written separately for each class. So pass the data through to that!

auto msg = Factory::Create(packet_data);

figures out the type from the data, and then calls the proper

unique_ptr<MBXGenericPacket> WhateverClass::CreatePacket(PacketData);

the only difference from what you have being that CreatePacket takes a parameter now.

Within the functions:

//static 
unique_ptr<MBXGenericPacket> WhateverClass::CreatePacket(PacketData data)
{
auto self= make_unique<WhateverClass>();
self->Deserialize(data);
return self;
}

The forth point too!

With the above design, nobody needs to be exposed to the type ID other than the factory function itself and each class which knows to initialize itself with the proper value when creating an empty message for sending.

Advanced

From my generic CreatePacket above, you can see that each one is exactly the same except for the name of its own class.

This suggests that you make it Generic for real. You only need one template function.

The only reason I say this is advanced is because of how you’re dispatching on the type to registered functions. You might scratch your head for a moment to figure out how to take the address of a specific instantiation. Then, you wonder why you need all those lines to populate the map when they are all the same except for the type name...