I'm working on a network application that implements a custom protocol. I want to easily extend the application support over the protocol as it changes. Also, as the application extends, I might need to implement other protocols. So I need a design that allows flexible packet creation, based on protocol and variety of packets that it supports.

I found that the best way to achive this is by using the Factory Pattern. (Or, for more than one protocol, an Abstract Factory).

Here is my implementation:

IPacket.h:

#pragma once
#include "PacketData.h"

class IPacket

public:
 // Deleted fuctions
 IPacket(const IPacket&) = delete;

 IPacket& operator=(const IPacket&) = delete;
public: 
 IPacket();
 virtual ~IPacket();

 // Encode data into a byte buffer
 virtual void Serialize(PacketData &) = 0;

 // Decode data from a byte buffer
 virtual void Deserialize(const PacketData &) = 0;
;

This provides a packet interface that will be used for further implementations of protocols.

MBXGeneric.h:

#pragma once
#include "IPacket.h"
#include "MBXHeader.h"
#include "MBXPacketFactory.h"

// Base class for custom protocol 
class MBXGenericPacket : public IPacket

public:
 MBXGenericPacket(MBXPacketFactory::MBXPacket _type) : H_MBX(static_cast<uint16_t>(m_Type))
 

 // Serialize MBXHeader
 virtual void Serialize(PacketData& data) override
 
 data.Data << H_MBX.h_time << H_MBX.P_Type;
 

 // Extract MBXHeader
 virtual void Deserialize(const PacketData& data) override
 
 data.Data >> H_MBX >> P_Type;
 

 MBXPacketFactory::MBXPacket GetType() const return static_cast<MBXPacketFactory::MBXPacket>( H_MBX.P_Type ); 

 static std::unique_ptr<IPacket> CreateGenericPacket(const MBXPacketFactory::MBXPacket Type)
 
 return std::make_unique<MBXGenericPacket>(Type);
 
protected:
 MBXHeader H_MBX;
;

This is the base class of an example protocol.

The packets have the following structure:

|MBX Header (Metadata + PacketType) | BODY |

The header will be initialized with each packet derrived from MBXGenericPacket. It can then be decoded or encoded in a byte buffer.

MBXPacketFactory.h:

#pragma once
#include <map>
#include <memory>
#include <mutex>
#include "MBXGeneric.h"
#include "Packets.h"

using CreateMBXPacketCb = std::unique_ptr<MBXGenericPacket>(*)();

class MBXPacketFactory

public:
 enum class MBXPacket : uint16_t
 
 MBXGeneric,
 Msg_MessageBox,
 ErrorResponseMsg,

 MAX_PACKET
 ;

private:
 using PacketsMap = std::map<MBXPacketFactory::MBXPacket, CreateMBXPacketCb>;

 PacketsMap map;
private:

 MBXPacketFactory()
 
 map[MBXPacket::Msg_MessageBox] = ChatMessage::CreatePacket;
 map[MBXPacket::ErrorResponseMsg] = ErrorResponse::CreatePacket;
 
public:
 MBXPacketFactory(const MBXPacketFactory&) = delete;
 MBXPacketFactory & operator=(const MBXPacketFactory&) = delete;
public:

 bool UnRegisterPacket(MBXPacketFactory::MBXPacket Type)
 
 return map.erase(Type);
 

 bool RegisterPacket(const MBXPacketFactory::MBXPacket& type, CreateMBXPacketCb callback)
 
 return map.insert(std::make_pair(type, callback)).second;
 

 std::unique_ptr<MBXGenericPacket> CreatePacket(MBXPacketFactory::MBXPacket Type)
 
 auto it = map.find(Type);

 if (it == map.end())
 throw std::runtime_error("Unknown packet type!");

 return it->second();
 

 static MBXPacketFactory& Get()
 
 static MBXPacketFactory instance;
 return instance;
 
;

Here's an example of packets:

// Packets.h
#include "MBXGeneric.h"
#include "MBXPacketFactory.h"

class ChatMessage : public MBXGenericPacket

public:
 ChatMessage() : MBXGenericPacket(MBXPacketFactory::MBXPacket::Msg_MessageBox)
 

 void Serialize(PacketData& data) override
 
 data << H_MBX;

 data << sz_Title << s_Title;
 data << sz_Message << s_Message;
 

 void Deserialize(const PacketData& data) override
 
 // Extract the header
 data >> H_MBX;

 data >> sz_Title;
 data.read(s_Title, sz_Title);

 data >> sz_Message;
 data.read(s_Message, sz_Message);
 

 static std::unique_ptr<MBXGenericPacket> CreatePacket()
 
 return std::make_unique<ChatMessage>();
 
public:

 // Getters and setters

 std::string GetTitle()
 
 return s_Title;
 

 std::string GetMessage()
 
 return s_Message;
 

 void SetTitle(const std::string& title)
 
 s_Title = title;
 

 void SetMessage(const std::string& message)
 
 s_Message = message;
 


private:
 int32_t sz_Title, sz_Message;

 std::string s_Title, s_Message;
;



class ErrorResponse : public MBXGenericPacket

public:
 ErrorResponse() : MBXGenericPacket(MBXPacketFactory::MBXPacket::ErrorResponseMsg)
 

 void Serialize(PacketData& data) override
 
 data << H_MBX;
 data << sz_Error << s_ErrorMessage;
 

 void Deserialize(const PacketData& data) override
 
 data >> H_MBX;

 data >> sz_Error;
 data.read(s_ErrorMessage, sz_Error);
 

 static std::unique_ptr<MBXGenericPacket> CreatePacket()
 
 return std::make_unique<ErrorResponse>();
 
public:

 std::string GetErrorMessage()
 
 return s_ErrorMessage;
 

 void SetErrorMessage(const std::string& msg_error)
 
 s_ErrorMessage = msg_error;
 

private:
 int32_t sz_Error;
 std::string s_ErrorMessage;
;

Example usage of factory:

When receiving a packet:

...

PacketData NewPacket = connection.RecvPacket();

uint16_t PacketID = GetUint16(NewPacket.Data);

std::unique_ptr<MBXGenericPacket> Packet = MBXPacketFactory::Get().CreatePacket(static_cast<MBXPacketFactory::MBXPacket>(PacketID));

// Up-cast the packet pointer to the right packet type

// Let's asume we've received an error response

std::unique_ptr<ErrorResponse> ErrorPacket(reinterpret_cast<ErrorResponse*>( Packet.release() ));

ErrorPacket->Deserialize(NewPacket);

std::cout << ErrorPacket->GetErrorMessage() << 'n';

...

When sending a packet:

...

std::string Title = GetUserInput();
std::string Message = GetUserInput();

std::unique_ptr<MBXGenericPacket> Interface = MBXPacketFactory::Get().CreatePacket(MBXPacketFactory::MBXPacket::Msg_MessageBox);

std::unique_ptr<ChatMessage> MessagePk(reinterpret_cast<ChatMessage*>( Interface.release() ));

MessagePk->SetTitle(Title);
MessagePk->SetMessage(Message);

PacketData MsgPacketData;
MessagePk->Serialize(MsgPacketData);

PacketQueue.push(MsgPacketData);

...

Improvements:

What I would like to improve in the current design:

• The down-casting from the base class to the derived class in
  order to access member functions.


• With each new packet derived from MBXGenericPacket, I'm forced to
  encode and decode the MBX_Header in the
  serialization/deserialization, thus causing code repetition.


• To get the required packet class from the factory, I need to first
  extract the identifier from the header and then pass it to the
  CreatePacket() method. I think it will be nice if you could just
  pass the PacketData to the factory, rather then decoding
  information by yourself.


• The factory's purpose is to abstract the creation process of classes.
  In my opinion, the packet identifiers should be encapsulated by the
  factory, but that causes a lot of noise in the code, as you have
  to type: MBXPacketFactory::MBXPacket:: ...
  

Let me know your thoughts about the code. I'm here to learn, so any opinion is welcomed !

This is rather long, so I’m posting a new answer rather than making it a P.S. on the first note about visitors.

You have separate IPacket (interface-only) and MBXGenericPacket (any kind of packet). I use only one class, as I want my typeID to be present in the polymorphic pointer. I don't like how you have two classes — which one am I supposed to use, and is it proper to use one or the other in different circumstances? I think it's an artifact of thinking of Java style interfaces.

Your post initially confused me because you use message and packet as synonyms. Packet is what comes over the wire, Message is the useful object you extract from it. So I'll go with Message here.

enum MBXHeader 
 eChat = 0,
 eError,
 // etc.
;


class Message 
protected:
 MBXHeader H_MBX; // packet type ID
 Message (MBXHeader id) : H_MBX(id) 
public:
 MBXHeader get_header() const return H_MBX; 
 virtual ~Message() = default;
 virtual void serialize(PacketData&) = 0;

 // this is your factory (needed for receiving)
 static unique_ptr<Message> Create (PacketData& data);
;

Your values in the chat message are free to set and get; they behave as a simple struct. So hiding them and having get/set for each one is not useful. If you have boilerplate get/set for every data member that do nothing other than return and assign, you are missing the point. In this case, you could have a content sanity check done before the serialization, if you wanted to check the values for legality.

class ChatMessage : public Message 
public:
 int32_t sz_Title;
 int32_t sz_Message;
 std::string s_Title;
 std::string s_Message;

 ChatMessage() : Message(eChat) 
 ChatMessage (PacketData& data);
 void serialize(PacketData&) override;
;

class ErrorMessage : public Message 
public:
 int32_t sz_Error;
 int32_t sz_Message;

 ErrorMessage() : Message(eError) 
 ErrorMessage (PacketData& data);
 void serialize(PacketData&) override;
;

The send side is simple and look at that first so it doesn't get over complicated with stuff you need for polymorphic creation and run-time types.

Just create a message object, populate it, and send it.

 ChatMessage msg; // no pointers!!
 msg.sMessage= "Hello World!";
 // ... etc.
 PacketData MsgPacketData;
 msg->serialize(MsgPacketData);
 PacketQueue.push(MsgPacketData);

To receive a message, you don't know the type. That's where you need a factory.

 PacketData NewPacket = connection.RecvPacket();
 auto incomingMessage = Message::Create (NewPacket);

Now what do you do with it? The easy way:

 incomingMessage->process(); // virtual function call

since that is what the program is for, essentially, I don't see what's wrong with making that a virtual function. Note that you can define them in different CPP files so the header doesn't need to know about the details of processing.

But this is just a toy example, and I know from my own work that we do indeed get different things we want to do and isolate the different systems from each other. So, as your original question indicated, you want to use the Visitor pattern.

Let’s start with how you want to use it, then go back and make it work.

As explained before, you define your "overloaded" function for each type you want to handle. The details of what you want to do go in the function bodies, not shown. Each body gets the parameter of the correct type.

struct process_message 
 void operator()(const ChatMessage&) const;
 void operator()(const ErrorMessage&) const;
;

Then have some common dispatching infrastructure that gets the object to the right function based on the actual type.

 visit (process_message(), *incomingMessage);

Just how do you write the visit function? We already looked at double-dispatch and are not doing that.

Note that we are assigning our own type ID codes, which is a fundimental part of having a wire encoding of different packets. That’s what we need for the protocol and ultimatly how the receiver does in fact know the type. So just use that! Unlike the C++ typeid, these are a compact range of integers so a switch/case statement is possible and very efficient.

 template<typename V>
 void visit (V visitor, Message& m)
 
 switch (m.get_header()) 
 default: throw std::invalid_argument("oops!");
 case eChat: 
 auto& msg = static_cast<ChatMessage&>(m);
 visitor(msg);
 break;
 
 case eError: 
 auto& msg = static_cast<ErrorMessage&>(m);
 visitor(msg);
 break;
 
 // repeat the cases for each kind of message.
 // Note: if checking for non-final types, order matters!
 
 

Piece of cake, though you have to replicate the small case clause by hand.

Now, what about reading? You’ll notice that the classes reflect my earlier comments in that the deserialization is done by the constructor. It can be implemented in the same manner as the visitor.

 unique_ptr<Message> Message::Create (const PacketData& data)
 
 uint16_t PacketID = GetUint16(data.Data);
 switch (PacketID) 
 default: throw std::invalid_argument("oops!");
 case eChat:
 return make_unique<ChatMessage>(data);
 case eError:
 return make_unique<ErrorMessage>(data);
 
 // add a case for each message type
 
 }

And that's all. The Create function is special, and then the visit function can handle everything else you want to do with the objects based on the concrete type of what was read.

For the first point — casting from the base class for every use.

really need that?

In the last block of example code, you are creating a packet of a type you do in fact know in advance — you are asking for a ChatMessage so you can populate it. So, don’t use the factory here! Just create the object.

Have the class provide a static member for that, so they are always created the right way (on the heap).

auto MessagePk = ChatMessage::Create();
MessagePk->SetTitle(Title);
 ⋮
// prep and send it.

Visitor Pattern

In the penultimate code block,
you are getting a generic (polymorphic) packet and need to do different things based on its type, but you are a consumer of the class and not in a position to add virtual functions to it.

What you want here is the visitor pattern.

Write a struct that is a function-callable object, but contains overloads for the different types of packet that you have. Then a single “don't look under the hood” piece of code will call the proper one based on the dynamic type. You can easily extend this with new “externally polymorphic” functions, and not write a single cast.

auto Packet= MBXPacketFactory::Get_Next_Packet(); // whatever
Packet.Deserialize(NewPacket); // this one should be a simple virtual function.
visit (my_processor, *Packet);

 ⋮

struct my_processor 
 void operator() (ChatMessage& packet);
 void operator() (ErrorMessage& packet)
 
 std::cout << packet->GetErrorMessage() << 'n';
 
 // etc.
;

Now how do you write visit? You can include a virtual function in the class hierarchy that does this. This is a classic approach, but it has the drawback that the processor has to inherit from an interface class and all the individual functions have to be virtual.

class Base 
 virtual void visit (const VisitInterface&) =0;
 

class Derived_1 : public Base {
 void visit (const VisitInterface& visitor)
 
 visitor (*this);
 

This is called double dispatch because you are using virtual calls twice to resolve two different polymorphic aspects of the call. The first call to visit is a virtual call so it recovers the correct type of the message object naturally. Then the body of that function uses overloading to call the proper function of the set. But, to allow for different commands, that requires a virtual dispatch too.

You also see that my sketch did not provide additional arguments to the call. If you had commands with different signatures, this gets repetitive.

Look at std::variant and the original Boost.Variant which has a good overview and tutorial about it.

This lets you write your visitor similarly to what I described: a single function-call object with overloaded forms in it. But it does not use polymorphism to do its work! The different classes are named explicitly in the definition of the variant.

You could write your program so that a generic message is a variant of the different kinds of messages, rather than a base class.

Then my example above would work as I wrote it by calling std::visit.

For the second point — code repetition of MBX_Header

It is only one line, and it should be. Everything that is common should go into the base class, so you only need one call to trigger it all.

It is not at all uncommon to have functions call their base class’s version first (or last), but I argue that this would be more cumbersome than what you have:

void Serialize(PacketData& data) override

 MBXGenericPacket::Serialize(data);
 data << sz_Error << s_ErrorMessage;

Using the same syntax to serialize or deserialize the common header, as you have it in your code, is intuitive and easy to understand.

The key is to make it one thing, as you have already done by having a single base member. If there were more data members there, you would group them in a struct or have a base class function to call with straightforward syntax.

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…