Serializable and extensible API implementing the Command Design Pattern

Question

I am currently trying to implement an extensible way to create commands that can be sent to some type of executor. In my case, there would be two of them (server and client). As I posted here, I had a hard time developping my idea, but it finally came through something usable, but a bit complex in my opinion.

I am thus looking for advice, and maybe ideas that could make this code better!

There is a working example at the last file for anyone wondering*.

protocol.hpp

#ifndef PROTOCOL_HPP_
#define PROTOCOL_HPP_

#include <cstdint>
#include <vector>
#include <memory>

#define MAX_COMMAND_BODY_LENGTH 512



namespace nw {



class command_base {

  public:
    using id = uint8_t;
    using buffer = std::vector<uint8_t>;

    static constexpr size_t header_length{sizeof(id)};
    static constexpr size_t max_body_length{MAX_COMMAND_BODY_LENGTH};

  protected:
    // We don't want to instanciate this class
    ~command_base() {}

};



template<class Executor>
class command: public command_base {

  public:

    using executor_t = Executor;
    using pointer = std::unique_ptr<command>;

    // This one has to be specialized later (when creating the executors)
    enum class command_t: id;

    command() = delete;
    explicit command(id);

    static pointer unserialize(buffer);
    buffer const serialize() const;

    virtual void execute(executor_t&) = 0;


  protected:

    id id_;

    buffer const serialize_header() const;

    // implementations will need to define this, 
    // depending on the member variables of the 
    // subclass.
    virtual buffer const serialize_body() const = 0;


  private:

    static pointer do_make_command(
      id const, buffer::const_iterator, buffer::const_iterator
    );

};



} // namespace nw


#include "protocol.ipp"

#endif // PROTOCOL_HPP_

protocol.ipp

#include <stdexcept>
#include <algorithm>
#include <cstring> // memcpy


namespace nw {



template<class T>
command<T>::command(id id): id_(id) {}



template<class T>
command<T>::pointer command<T>::unserialize(buffer buffer) {

    auto it{buffer.cbegin()};

    if (std::distance(it, buffer.cend()) > header_length + max_body_length) {
        throw std::invalid_argument(
            "command::unserialize: buffer is too long."
        );
    }

    command::id id;
    std::copy(it, it + sizeof(id), &id);
    it += sizeof(id);


    return std::move(do_make_command(id, it, buffer.cend()));

}



template<class T>
command_base::buffer const command<T>::serialize() const {

    buffer header{serialize_header()};
    buffer body{serialize_body()};

    if (body.size() > max_body_length) {
        throw std::invalid_argument("command::serialize: buffer is too long.");
    }

    // Join the two buffers into one
    buffer res;

    res.resize(header.size() + body.size());

    auto it{res.begin()};

    std::copy(header.cbegin(), header.cend(), it);
    it += header.size();
    std::copy(body.cbegin(), body.cend(), it);

    return res;

}



template<class T>
command_base::buffer const command<T>::serialize_header() const {

    buffer buff(header_length);
    auto it{buff.begin()};

    std::memcpy(&*it, &id_, sizeof(id_));
    it += sizeof(id_);

    // Normally not necessary, but better safe than sorry
    buff.shrink_to_fit();

    return buff;

}



template<class T>
command<T>::pointer command<T>::do_make_command(
    id const                id,
    buffer::const_iterator  beg,
    buffer::const_iterator  end
) {

    switch(static_cast<command_t>(id)) {

        default: throw std::invalid_argument(
            "command::unserialize: Unknown server command ID."
        );
    }

}



} // namespace nw

command_example.cpp

#include "protocol.hpp"
#include <iostream>
#include <string>



namespace nw {


// Create an executor that can handle the commandsthat are gonna be implemented
// (could be a server or a client, for example)
class MyExecutor {

  public:
    // This one doesn't do much....
    void print(std::string msg) {
        std::cout << msg << std::endl;
    }

};


// We need to specify what commands are gonna be usable from that executor
// We thus specialize the command template with the enum
template<>
enum class command<MyExecutor>::command_t: command_base::id {MessageCommand};



// We define a simple command that prints a message through the executor
class MessageCommand: public command<MyExecutor> {
  public:

    // First, we need to construct the command, we assign its ID,
    // then do stuff with the buffer (here it only contains a string,
    // that we store directly in a variable. It is EXTREMELY important
    // that we have at least this constructor with this signature,
    // since it's the one used by unserialize
    MessageCommand(buffer::const_iterator b, buffer::const_iterator e):
        command(static_cast<id>(command_t::MessageCommand)),
        msg(b, e)
    {}

    // Feel free to make other constructors
    MessageCommand(std::string s):
        command(static_cast<id>(command_t::MessageCommand)),
        msg(s)
    {}

    // Simple execute function
    virtual void execute(MyExecutor& e) {
        e.print(msg);
    }

  protected:

    // finally, we need to create the body serializing method.
    // This one is easy, since it can directly store the only
    // variable it holds
    virtual buffer const serialize_body() const {
        return buffer(msg.cbegin(), msg.cend());
    }


    std::string msg;

};

// This I'm really not sure... How do I define the specialized
// do_make_command method???
// That way seems too verbose. I guess I will only have to
// do that twice (Server/Client) so it doesn't really matter

// added later: I could probably just directly make do_make_command virtual,
// and implement it completely.Some repeated code but whatever I guess.
template<>
command<MyExecutor>::pointer command<MyExecutor>::do_make_command(
    id const                id,
    buffer::const_iterator  beg,
    buffer::const_iterator  end
) {

    // This is always the same thing, we just add new cases
    // when adding new commands... This is the part I like the least.
    // Could maybe make a Macro to add cases??? But Macros are baaad :(
    switch(static_cast<command_t>(id)) {

        case command_t::MessageCommand: return pointer(
            new MessageCommand(beg, end)
        );

        default: throw std::invalid_argument(
            "command::unserialize: Unknown MyExecutor command ID: "
                + std::to_string(id)
        );
    }

}



} // namespace nw

// Now it's super easy to use!
int main() {
    using namespace nw;

    // Instanciate executor
    MyExecutor exec;

    // Create some commands
    std::string my_message{"Hello!"};
    MessageCommand my_command(my_message);

    std::cout << "Executing from the manually constructed command:\n";
    my_command.execute(exec);


    // Can serialize/unserialize to be sent through some stream/socket,
    // for example.
    std::cout << "\nExecuting from serialized then unserialized command:\n";
    command<MyExecutor>::pointer c{
        command<MyExecutor>::unserialize(
            my_command.serialize()
        )
    };


    c->execute(exec);


    return 0;
}

Basically, having to specialize the enum template seems a bit verbose... Could I instead have a second template argument which is an enum when defining command?

template<class Executor, class Enum>
class command {
    // ...
};

Is there anything else that I could do to improve this design?

NB: This is the start of my first real more ambitious project in C++, so if there is any cue about my style/things I do wrong, I am open to criticism

Thank you!

Answer 1

using id = uint8_t;
using buffer = std::vector<uint8_t>;

static constexpr size_t header_length{sizeof(id)};
static constexpr size_t max_body_length{MAX_COMMAND_BODY_LENGTH};

Misspelt std::uint8_t and std::size_t here - that's a lurking portability bug (you're getting away with this because your current compiler is exercising its option to declare global-namespace copies of standard identifiers, but will fail on compilers that don't do that).

command_base is intended to be a base class, so its destructor should be virtual.

Given that buffer is a std::vector and it is buffer.cbegin(), we can replace std::distance(it, buffer.cend()) with a simple buffer.size().