First things first, I'd thank Mr. G. Sliepen and Mr. Davislor regarding their help in the previous questions (C++ System data transfer design) (C++ System data transfer design - Following 1), given the resultant code can be found here*.

* The code includes some added functionalities beyond the point :)

Now I'm taking the review request one step further, as I've mentioned the "simplified SystemInterface" within the last question, I'm here to focus on it with any potential enhancements and optimizations.

General notes:

  • The System Data transfer is over-simplified within this code.
  • An introduced "Guarders" to guard against inaccepted payloads and allowed for correction in some cases.
  • A partial of SystemInterface is herein provided.
  • Comes with a "simplified ServiceManager", which manages Services (or Entities), giving each one a unique number which is utilized to identify it across other entities (as Logger/SystemInterface/..).
  • SystemInterface is provided here as one instance lies in the global scope, in my code it's a composed object within each entity, which receives a pid at construction.
  • The SystemInterface uses a kind of structural recursion to find the intended request.

Generally _execReq is called when a data is received by any communication mean (as MQTT/HTTP/Serial/...), and it's callable from other entities ofcourse.

One of my concerns is the runtime of _execReq call, and whether it's already optimized or there's a room for optimizing.

#include <algorithm>
#include <cassert>
#include <concepts>
#include <cstdint>
#include <functional>
#include <iostream>
#include <iterator>
#include <ranges>
#include <span>
#include <string>
#include <string_view>
#include <unordered_map>
#include <utility>
#include <variant>
#include <vector>

//  Tools

#define CHOP_FRONT(vs) (std::vector<std::string>(++vs.begin(),vs.end()))

std::string join(const std::vector<std::string>& vs,const char* delim) {
    std::string rv="";
        std::string sd(delim);
        for(auto const& v:vs) rv+=v+sd;
        for(int i=0;i<sd.size();i++) rv.pop_back();     
    return rv;

std::vector<std::string> split(const std::string& s, const char* delimiter){
    std::vector<std::string> vt;
    std::string delim(delimiter);
    auto len=delim.size();
    auto start = 0U;
    auto end = s.find(delim);
    while (end != std::string::npos){
        vt.push_back(s.substr(start, end - start));
        start = end + len;
        end = s.find(delim, start);
    std::string tec=s.substr(start, end);
    if(tec.size()) vt.push_back(tec);       
    return vt;

enum class RequestErrors : uint8_t


//  Data types
using BasicReferenceData = std::variant<std::string_view, std::span<std::uint8_t>>;
using BasicConcreteData = std::variant<std::string, std::vector<uint8_t>>;
std::span<std::uint8_t> t;
using ConcreteDataContainer = std::vector<BasicConcreteData>;
using ReferenceDataContainer = std::vector<BasicReferenceData>;

    Over simplified ReferenceData and ConcreteData, for the full implemented: https://godbolt.org/z/EofKMzGE9
using ReferenceData = ReferenceDataContainer;
using ConcreteData = ConcreteDataContainer;

using ReturnData = ConcreteData;
using RequestReturn = std::pair<RequestErrors, ReturnData>;
using CallbackSignature = std::function<RequestReturn(const std::string &context, const ReferenceData &)>;

///     Guarders
using PayloadGuardFN = std::function<RequestReturn(const ReferenceData&)>;

/*      Guarders Helper functions    */

bool binaryData(const BasicReferenceData& data) {
    return std::holds_alternative<std::span<std::uint8_t>>(data);
bool printable(const BasicReferenceData& in) {
    auto span = std::get_if<std::span<std::uint8_t>>(&in);
    return !span || std::all_of(span->begin(), span->end()-1, isprint) && (isprint(span->back()) || span->back() == '\0'); // Allows the presence of zero terminator 
    // !span means it's a string_view
void to_string(BasicReferenceData& data) { // Might remove the check.
    if (std::holds_alternative<std::span<std::uint8_t>>(data)) {
        auto span = std::get<std::span<std::uint8_t>>(data);
        data = BasicReferenceData{std::string_view(reinterpret_cast<char*>(span.data()), span.size())};
void to_string(ReferenceDataContainer::iterator it) {
    auto span = std::get<std::span<std::uint8_t>>(*it);
    *it = BasicReferenceData{std::string_view(reinterpret_cast<char*>(span.data()), span.size())};
bool is_number(std::string_view s) {
    bool numeric = false;
    if (s.size()) {
        bool first_char_valid = numeric = (isdigit(s.front()) || (s.front() == '-' && s.size() > 1));
        if (first_char_valid) {
            bool found_dot = false;
            for (auto it = s.begin(); it != s.end(); it++) {
                if (isdigit(*it)) continue;
                else if (*it=='.' && !found_dot) {
                    found_dot = true;
                numeric = false;
    return numeric;

class Guarder {
    virtual RequestReturn operator()(const ReferenceData& in) = 0;

/*        Guarder classes       */
class CountGuarder : public Guarder { // Or a template ??
    size_t count;
            RequestReturn operator()(const ReferenceData& in) override{ return std::make_pair((in.size() > count ? RequestErrors::TOO_MANY_INPUTS : in.size() < count ?  RequestErrors::TOO_FEW_INPUTS : RequestErrors::OK), ReturnData{});}
    CountGuarder(size_t count) : count(count) {}

class StringsGuarder : public Guarder {
            RequestReturn operator()(const ReferenceData& in) override {
                std::cout << "String Guarder\t";
                auto mut_in = const_cast<ReferenceData*>(&in);
                // auto view = in | std::views::filter(binaryData);
                if (std::all_of(in.begin(), in.end(), printable)) {
                    for (auto it = mut_in->begin(); it != mut_in->end(); it++) {
                        if (binaryData(*it)) {
                    // std::ranges::for_each(mut_in | std::views::filter(binaryData), to_string);
                    std::cout << "OK\n";
                    return std::make_pair(RequestErrors::OK, ConcreteData{});
                } else {
                    std::cout << "NOT_STRING\n";
                    return std::make_pair(RequestErrors::NOT_STRING, ConcreteData{});

// Should follow a StringGuarder within the pipeline and remove the StringGuarder inside?
class NumericGuarder : public Guarder {
            RequestReturn operator()(const ReferenceData& in) override {
                std::cout << "Numeric Guarder\t";
                StringsGuarder stringify;
                auto result = stringify(in);
                if (result.first != RequestErrors::OK) {
                    return result;
                for (auto& i : in) {
                    auto* sv = std::get_if<std::string_view>(&i);
                    if (!sv || !is_number(*sv)){
                        std::cout << "Not Numeric!\n";
                        return std::make_pair(RequestErrors::NOT_NUMERIC, ConcreteDataContainer{std::string(*sv)});
                std::cout << "OK\n";

                return std::make_pair(RequestErrors::OK, ConcreteData{});

// Can build custom Guarders.

RequestReturn toStringCorrector(const ReferenceData& pload) {
    StringsGuarder stringify;
    return stringify(pload);

//  SystemInterface

struct request {
    std::uint32_t           owner;
    std::uint32_t           levID;
    CallbackSignature       cbf;
    std::vector<Guarder*>   guarders;

using   REQMAP          = std::unordered_multimap<std::string,request>;
using   REQMAP_I        = REQMAP::iterator;

enum REQ_ID {
    REQ_LOGGER, // A predefined ID for some instances, as the Logger.

class SystemInterface
    REQMAP          requestsMap;

    REQMAP_I __exactMatch(const std::string& cmd,uint32_t owner){
        auto any=requestsMap.equal_range(cmd);
        for(auto i=any.first;i!=any.second;i++) if(i->second.owner==owner) return i;
        return requestsMap.end();
    void __flatten(std::function<void(std::string)> fn){ // For generating "help" list
        REQMAP_I ptr;
        for(ptr=requestsMap.begin();ptr!=requestsMap.end(); ptr++){
                if(ptr->second.levID) _flattenCmds(fn,ptr->first,ptr->first,ptr->second.levID);    
                else fn(ptr->first);

    RequestReturn _dispatch(std::vector<std::string> vs, ReferenceDataContainer ploads, uint32_t owner=0){
        std::cout << "_dispatch: " << join(vs,"/") << "\n";
            REQMAP_I i;
            std::string cmd=vs[0];
                if(i->second.cbf) {
                    return [=]()->RequestReturn{ 
                        for(auto& guard : i->second.guarders) {
                            auto ret = (*guard)(ploads);
                            if (ret.first!=RequestErrors::OK) {
                                return ret;
                        return i->second.cbf(join(CHOP_FRONT(vs),"/"), ploads); 
                else return _dispatch(CHOP_FRONT(vs), ploads, i->second.levID);
            } else return std::make_pair(RequestErrors::NOT_FOUND, ConcreteDataContainer{});
        } else return std::make_pair(RequestErrors::NOT_FOUND, ConcreteDataContainer{});
    void addRequest(uint32_t owner, uint32_t levId, const std::string &name, CallbackSignature cbf, std::initializer_list<Guarder*> guarders={})
        if (__exactMatch(name, owner) == requestsMap.end()){
            // std::cout << "Inserting " << name << "\n";
            requestsMap.insert(std::make_pair(name,request{owner,levId, cbf, guarders}));}
            std::cout << "Already assigned path!\n";

    RequestReturn _execReq(std::string topic, ReferenceDataContainer ploads){
        std::vector<std::string> cmd=split(topic,"/");
        return _dispatch(cmd, ploads); // optimise?
    void _flattenCmds(std::function<void(std::string)> fn,std::string cmd,std::string prefix,uint32_t lev){
        REQMAP_I i=requestsMap.find(cmd);
                std::string trim = prefix+"/"+i->first;
                if(i->second.levID) _flattenCmds(fn,i->first,trim,i->second.levID);
                else fn(trim);

    void help() {
        std::vector<std::string> unsorted={};
        __flatten([&unsorted](std::string s){ unsorted.push_back(s); });
        std::cout << "HELP: \n";
        for (auto& path : unsorted) {
            std::cout << "\t" << path << "\n";


SystemInterface interface;

// Guarder objects

CountGuarder c1Guard(1);
NumericGuarder nGuard;
StringsGuarder sGuard;

// ServiceManager

class ServiceManager {
    std::uint32_t pid;
    std::string name;
    static std::uint32_t services; 
    ServiceManager(std::string name) : name(name), pid(++services+REQ_MAX) {
        // std::cout << "name=" << name << " pid=" << pid << "\n";
std::uint32_t ServiceManager::services=0;

class Service1 : public ServiceManager {
    Service1() : ServiceManager("service1") {
        // Installs a level of "svc1"
        interface.addRequest(REQ_ROOT, pid, "svc1", nullptr, {}); // LevelID
        // Can add sub level ID on top of (pid) further ...

        // Adds a request path "svc1/test1"
        interface.addRequest(pid, 0, "test1",[this](const std::string& ctx, const ReferenceData& pload){
            std::cout << "Received on test1\n";
            return std::make_pair(RequestErrors::OK, ConcreteDataContainer{std::string{"Hello from test1"}});
        }, {});

        // Adds a request path "svc1/onestring" 
        interface.addRequest(pid, 0, "onestring",[this](const std::string& ctx, const ReferenceData& pload){
            std::cout << "Received on onestring\n";
            return std::make_pair(RequestErrors::OK, ConcreteDataContainer{std::string{"Hello from onestring"}});
        }, {&c1Guard, &sGuard});

class Logger : public ServiceManager {
    // dummy logger
    int level;
    Logger() : ServiceManager("logger") {
        interface.addRequest(REQ_ROOT, REQ_LOGGER, "logger", nullptr, {}); // LevelID

        interface.addRequest(REQ_LOGGER, 0, "level", [this](const std::string& ctx, const ReferenceData& pload){
            std::cout << "Received on level ctx=" << ctx << "\n";
            if (ctx == "set") {
                if ((pload.size() == 1)) {
                    if (auto plevel = std::get_if<std::string_view>(&pload[0])) { // MUST be true with the sGuard.
                        level = std::atoi(std::string(*plevel).c_str());
                } else {
                    return std::make_pair(RequestErrors::TOO_FEW_INPUTS, ConcreteDataContainer{});
            } else if (ctx != "get") {
                return std::make_pair(RequestErrors::NOT_FOUND, ConcreteDataContainer{});
            return std::make_pair(RequestErrors::OK, ConcreteDataContainer{std::to_string(level)});
        }, {&sGuard, &nGuard});

Service1 svc1;
Logger logger;

And here's a compiler explorer link with a simple example.

Lastly, I'd like to attribute Philbowles (Passed) who is the original author of SystemInterface's internal methods which inspired me.

With Thanks,
Hamza Hajeir

  • 1
    \$\begingroup\$ Not a full review, but the first thing that jumps out at me right away is: avoid C-style macros if at all possible. CHOP_FRONT should be a constexpr function, possibly returning a std::span. \$\endgroup\$
    – Davislor
    Sep 18 at 14:47
  • \$\begingroup\$ I’d also call the slice it computes the tail. \$\endgroup\$
    – Davislor
    Sep 18 at 14:51
  • \$\begingroup\$ Understood for the macro issue, I've implemented elsewhere a templated version of it. Per the span, I will consider it as it will avoid copies.... \$\endgroup\$ Sep 18 at 16:54
  • \$\begingroup\$ Didn't understand clearly the last comment, sorry. \$\endgroup\$ Sep 18 at 16:54
  • \$\begingroup\$ I meant, if I were writing a function that generated a subrange of all but the first element of a range, I would usually call it, tail. (But you can tell from my answers here that my coding style is heavily influenced by functional programming.) I would also make it a view, not copy every element. \$\endgroup\$
    – Davislor
    Sep 18 at 19:53

1 Answer 1


Avoid macros whenever possible

Macros are notoriously difficult to write correctly, and will easily fail with different use cases. For example, the following will fail to compile:

CHOP_FRONT(std::vector<std::string>{"Aap", "Noot", "Mies"});

If you had written a regular function, as already suggested by Davislor, you would have avoided this issue.


I strongly recommend you use verbs for functions, and nouns for types and variables. For example, isBinaryData() and isPrintable().

Be consistent: why use snake_case in to_string(), but camelCase in toStringCorrector()? Why does struct request not start with a capital when other types do? Why are some type aliases in ALL_CAPS?

Never use double underscores: these are always reserved. I also recommend not using leading underscores at all. Either use trailing underscores (1 at most), or use some other prefix, like m_ for class members.

What is the significance of a leading underscore anyway? I see you have some public member functions also starting with an underscore, so it definitely doesn't mean "private", otherwise you could have made those functions explicitly private.

Incorrect error handling

I see a lot of information messages, warnings and error messages all being written to std::cout. However, all this should go to std::log and std::cerr as appropriate.

Furthermore, if you encounter an error, you must do something about it. Just printing something like "Already assigned path!\n" is not enough; the program will happily continue running, the user looking at the output thinks "What path? Is this good or bad? I'll just ignore it." Consider throwing an exception; either this will cause the program to abort, or the caller can catch it if it has some way to recover from this error. If you really can't use exceptions, use some other way to panic or to return the error to the caller somehow.

This is unnecessarily complex

There are several issues with this design that make it unnecessarily complex. Let's start with the levels and owners. Why have these at all? Why split the command path into its components, and then have a std::unordered_multimap sorted by components, when you can just have a map ordered by the full request path? I think you can greatly simplify things by writing:

class SystemInterface
    std::unordered_map<std::string request, CallbackSignature> requestsMap;
    void addRequest(const std::string& path, CallbackSignature cbf) {

Another issue are the guarders. It might seem like this is convenient, so the callback functions don't have to validate the payload. But you could easily rewrite the guarders so the callback functions can call them, such that the code is just as short as with your solution. Consider:

interface.addRequest("logger/level", [this](auto& ctx, auto& pload) {
    if (auto plevel = GetNumber(pload[0])) {
        level = *plevel;

Where you created helper functions to deal with the data encapsulated in BasicReferenceData:

std::optional<int> GetNumber(const BasicReferenceData& data) {
    if (/* checks to see it data actually contains a valid number */) {
        return std::atoi(std::get<std::string_view>(data));
    } else {
        return std::nullopt;

With C++23 you will get std::expected<>, so you can also have it return a more specific error code. Your RequestReturn type is a bit similar in that it holds some desired data and a possible error code, but it's hardcoded to return ConcreteData, which still requires the caller to further unpack that, and it doesn't have any of the handy features of std::optional and std::expected, like conversion to bool returning if it has an expected value or not, as well as monadic operations since C++23. If you cannot use that yet but still want to return something like RequestReturn, consider making that a proper class that emulates std::expected

Anyway, with these changes, SystemInterface is greatly simplified: it mostly is a std::unordered_map with some access control. help() no longer needs to flatten anything, as the paths are already stored flattened.

  • \$\begingroup\$ Thanks for your review, you're correct in most of the points. Per the macros I'll change them, probably with changing the std::vector accepting _despatch function to std::span one. Per naming and code style I agree and I will unify them. Per the underscore issue, it's for the private member functions, with my framework I do respect this style, but tend to copy from that library for the simplicity of the functions. This applies for the addRequest error handling, an error would be returned. \$\endgroup\$ Sep 21 at 15:43
  • \$\begingroup\$ Per the multimap and this algorithm, I found it necessary somehow, as it reduces the memory footprint a log, consider lots of multilevel (3-5) paths, beside that this allows for "context" passing, as the logger has a "level" resource, but also allowed for sub-categorize endpoints ("set" and "get") held by the std::string ctx parameter. Generally I believe it's a tradeoff issue between memory and performance somehow. Thanks for mentioning helper functions concept and std::expected<>. I'll study them very soon. \$\endgroup\$ Sep 21 at 15:52
  • \$\begingroup\$ @HamzaHajeir There is indeed a tradeoff, but I think that you underestimate the memory overhead of having multiple multimap nodes per path. Consider benchmarking the memory usage with a realistic set of paths added to the SystemInterface. \$\endgroup\$
    – G. Sliepen
    Sep 21 at 16:34
  • \$\begingroup\$ Given that issue taken away, how do you think I can bypass the second issue (correct separation between the URL resource and the "context")? \$\endgroup\$ Sep 23 at 10:41
  • \$\begingroup\$ Per std::expected it's somehow similar to std::variant with an error code handling, with errorcode it's more convinient and perhaps more optimized for the purpose. Until now the framework needs to return both ConcreteData and errorcode in some cases. Per emulating std::expected within the ConcreteData class, there I've switched to inheriting from std::vector<BasicConcreteData> to remove the std API boilerplate, in which relucts the adoption of std::expected or IMO the more suitable std::optional. \$\endgroup\$ Sep 24 at 8:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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