4
\$\begingroup\$

I'm writing a typical client-server architecture for a chat program. My goal is to make the protocol flexible so that I could add more functionality in the future. (Any good software should allow that)

Both applications should be able to wait for a response from a particular request that has been sent.

I'm currently using two main design patterns in my code:

  • Command Pattern - Requests and responses will be encapsulated as objects
  • Visitor Pattern - To process commands

Command pattern:

Each command inherits from the ChatPacket base class:

// chat_packet.hpp
#include "Processor/IProcessor.hpp"

class ChatPacket
{
public:
    ChatPacket() = default;

    ChatPacket(uint8_t type)
        : pk_type(type)
    {}

    template <class Archive>
    void serialize(Archive& arc)
    { arc(pk_type); }

    virtual void process(IProcessor& proc) = 0;
    virtual ~ChatPacket() {}
protected:
    enum PacketType : uint8_t
    {
        Chat_Control,
        Chat_Error,
        Chat_Message,

        MAX_PKTYPE
    };
protected:
    uint8_t pk_type;
};

I've divide commands in categories for easy management. Each base category could be divided into more subcategories and so on, thus creating hierarchies of commands, as you can see in the following example:

// chat_control.hpp
#include "chat_packet.hpp"

class ChatControl : public ChatPacket
{
public:
    ChatControl(uint8_t control_msg)
        : ChatPacket(Chat_ControlMsg),
          ctrl_type(control_msg)
    {}

    ChatControl() : ChatPacket(Chat_ControlMsg)
    {}

    virtual void process(IProcessor& proc){
        proc.process(*this);
    } 

    template <class Archive>
    void serialize(Archive& arc)
    { arc(ctrl_type);}
protected:
    enum CtrlMessages : uint8_t
    {
        Ctrl_Kick,
        Ctrl_MakeAdmin,
        Ctrl_CloseChatRoom,
        Ctrl_ListUsers,

        MAX_CTRL_MSG
    };
protected:
    uint8_t ctrl_type;
};

class KickUser : public ChatControl
{
public:
    KickUser(const std::string& user)
        : ChatControl(Ctrl_Kick), user_to_kick(user)
    {}

    KickUser() : ChatControl(Ctrl_Kick)
    {}

    template <class Archive>
    void serialize(Archive& arc)
    { arc(user_to_kick); }

    void process(IProcessor& proc) override{
        proc.process(*this);
    }
public:
    std::string user_to_kick;
};

class CloseChatRoom : public ChatControl
{
public:
    CloseChatRoom(const std::string& user)
        : ChatControl(Ctrl_CloseChatRoom), username(user)
    {}

    CloseChatRoom() : ChatControl(Ctrl_CloseChatRoom)
    {}

    template <class Archive>
    void serialize(Archive& arc)
    { arc(username);}

    void process(IProcessor& proc) override{
        proc.process(*this);
    }
public:
    std::string username;
};

class MakeChatAdmin : public ChatControl
{
public:
    MakeChatAdmin(const std::string& username)
        : ChatControl(Ctrl_MakeAdmin), user_to_admin(username)
    {}

    MakeChatAdmin() : ChatControl(Ctrl_Make_Admin)
    {}

    template <class Archive>
    void serialize(Archive& arc)
    { arc(user_to_admin);}

    void process(IProcessor& proc) override{
        proc.process(*this);
    } 
public:
    std::string user_to_admin;
};

class ListChatParticipants : public ChatControl
{
public:
    ListChatParticipants(const std::vector<std::string>& list)
        : ChatControl(Ctrl_ListUsers), user_list(list)
    {}

    ListChatParticipants() : ChatControl(Ctrl_ListUsers)
    {}

    template<class Archive>
    void serialize(Archive& arc)
    { arc(user_list); }

    void process(IProcessor& proc) override{
        proc.process(*this);
    } 
public:
    std::vector<std::string> user_list;
};

// chat_message.hpp
#include "chat_packet.hpp"

class ChatMessage : public ChatPacket
{
public:
    ChatMessage(const std::string& msg)
        : ChatPacket(Chat_Message), message_body(msg)
    {}

    ChatMessage() : ChatPacket(Chat_Message)
    {}

    template <class Archive>
    void serialize(Archive& arc)
    { arc(message_body); }

    void process(IProcessor& proc){
        proc.process(*this);
    } 
public:
    std::string message_body;
};

The current approach has a couple of downsides:

  • There's an explosion of classes, implicitly lots of functions to overload to support each type when processing is required. (A major problem when you try to implement the good old Visitor pattern)
  • Code repetition: the structure of classes is pretty much the same (You would expect this, as I use inheritance to define my commands)

Visitor Pattern

When having such a rich collection of commands (one class for each command), the visitor pattern makes you question your life. It's tricky to implement and has more downsides than advantages, but it's still widely used.
The implementation is the basic one. First the abstract visitor type:

// IProcessor.hpp 
#pragma once

class ChatMessage;


class ChatControl;
    class KickUser;
    class CloseChatRoom;
    class MakeChatAdmin;
    class ListChatParticipants;

class IProcessor
{
public:
    virtual void process(ChatMessage&) = 0;

    virtual void process(ChatControl&) = 0;
    virtual void process(KickUser&) = 0;
    virtual void process(CloseChatRoom&) = 0;
    virtual void process(MakeChatAdmin&) = 0;
    virtual void process(ListChatParticipants&) = 0;    
};  

Followed by concrete implementations of processors(or handlers):

//chat_control_processor.hpp
#pragma once
#include "IProcessor.hpp"
#include "chat_room.h"

class ChControlHandler : public IProcessor
{
public:
    ChControlHandler(ChatRoom& ch_room)
        : Room(&ch_room)
    {}
public:
    // This function does nothing, as
    // The type is not suported by the current handler
    void process(ChatMessage&) override;


    void process(ChatControl&) override;
    void process(KickUser&) override;
    void process(CloseChatRoom&) override;
    void process(MakeChatAdmin&) override;
    void process(ListChatParticipants&) override; 
private:
    std::unique_ptr<ChatRoom> Room;
};

Now you should see the main problem with the current design, and if you don't... check your glasses.

I'm using cereal library for serialization which automatically constructs objects from pointers to base class, which makes enums in base classes pretty much useless. I kept them just for logging purposes.

I'm looking for an alternative to the visitor pattern that will allow one handler per commands category and overall advice on the current design and program structure. Any suggestions on naming convensions, best practices and opinions on the code are welcomed!

\$\endgroup\$
  • \$\begingroup\$ I had to make lots of minor edits to improve this post's quality. If you want to revert any of them, feel free to edit the question again. In the future, you may want to do a quick spell-check before submitting a question. \$\endgroup\$ – Daniel Jun 2 '18 at 20:30
1
\$\begingroup\$

The problem with the Command Pattern is indeed the explosion of classes and the verbose boilerplate around each little function. That is better done today with lambda functions: the closure syntax automatically packs up the values in a struct and makes a callable operator() with the code body.

To use that at run-time (rather than templates), the code takes and manipulates std::function.

Sending a struct to match a process(Foo) is rather circumlocutory, which is probably what you are complaining about.

Foo command { param1, param2, param3 };
q->accept(command);
 // eventually causes a call to:
p->process(command);

 ⋮

void process (const Foo& args)
{
   do_it_for_real (args.param1, args.param2, args.param3);
}

Instead, just write the code you want:

auto command= [=]{ p->do_it_for_real (param1, param2, param3); }
q->accept (command);

and then processing just runs whatever is in that function object.

There are variations on what gets passed to the lambda when it is run vs. what is captured. But in general, a weak_ptr to the object of interest can be captured and the lambda is completely self contained, not needing any support from the class itself.

I hope that makes sense to you and gives you some ideas.

In one project, I replaced such command structs with a single ServiceCall template. The point there, IIRC, was to run the code on the proper thread. So, the lambda was the code you really wanted to do, ServiceCall queued it to the thread handling that object, using the same queue that was used for the polymorphic command objects before. Before, the class needed a function for the real thing to do, and a second function to respond to the matching command object, and the command object, and code to pack up params into it. Now, it just needs the function for the real thing to do.

\$\endgroup\$
  • \$\begingroup\$ If I understand correctly, I encapsulate the action in a lambda function and then I just call that action when I need to. All the arguments for the object will be captured in the lambda. That's a really powerful idea, but I don't think is going to work in my case. I need to send commands over the network ( That's the reason each command class has a serialization method ), from the client to the server and back. Would a map of requests to lamdas (actions) work? Is there a better solution? \$\endgroup\$ – Cosain Melic Jun 3 '18 at 11:28
  • \$\begingroup\$ You are right, a lambda will not serialize for passing between programs. What you need is an ID for each verb, and on the receiver side map those to actual calls (say, in a big switch statement). You can come up with generic argument handling, and not need an explosion of individual classes or virtual calls. \$\endgroup\$ – JDługosz Jun 3 '18 at 22:46
  • \$\begingroup\$ Generic argument handling will work, but for a serialization method that you can control. For me, cereal does all the work. I just need to write my data structure, serialize it and sent it over the wire. The problem I have is mapping those data structures to the right processing method. Visitor pattern I don't think will do the job unless I have a different visitor for each superclass of commands. ( ChatControl is a superclass that knows about all control commands ). This will allow me to have a handler for each superclass, but I don't see that scale very well. What do you say? \$\endgroup\$ – Cosain Melic Jun 4 '18 at 6:22
  • 1
    \$\begingroup\$ Serialize one structure: { int cmdID; vector<any> args; } or something like that. The receiver uses a switch statement based on cmdID to run the proper code. \$\endgroup\$ – JDługosz Jun 5 '18 at 0:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.