Skip to main content
added 20 characters in body
Source Link
G. Sliepen
  • 61.7k
  • 3
  • 61
  • 152
class WriteConfig {
    bool ordered; // default values not needed in this case
    std::int32_t maxTimeMS;
    …
public:
    // no friend declaration needed
    WriteConfig(bool ordered, std::int32_t maxTimeMS, …)
        : ordered(ordered)
        , maxTimeMS(maxTimeMS)
        , …
    {}
    …
};

class WriteConfigBuilder {
    bool orderedm_ordered = true;
    std::int32_t maxTimeMSm_maxTimeMS = 0;
    …
public:
    // rule of zero
    WriteConfig build() const {
        return WriteConfig(orderedm_ordered, maxTimeMSm_maxTimeMS, …);
    }
    WriteConfigBuilder& ordered(bool v)           { orderedm_ordered = v; }
    WriteConfigBuilder& maxTimeMS(std::int32_t v) { maxTimeMSm_maxTimeMS = v; }
    …
};
class WriteConfig {
    bool orderedm_ordered = true;
    std::int32_t maxTimeMSm_maxTimeMS = 0;
    …
public:
    // rule of zero
    WriteConfig& ordered(bool v)           { orderedm_ordered = v; }
    WriteConfig& maxTimeMS(std::int32_t v) { maxTimeMSm_maxTimeMS = v; }
    …
};
class WriteConfig {
    bool ordered; // default values not needed in this case
    std::int32_t maxTimeMS;
    …
public:
    // no friend declaration needed
    WriteConfig(bool ordered, std::int32_t maxTimeMS, …)
        : ordered(ordered)
        , maxTimeMS(maxTimeMS)
        , …
    {}
    …
};

class WriteConfigBuilder {
    bool ordered = true;
    std::int32_t maxTimeMS = 0;
    …
public:
    // rule of zero
    WriteConfig build() const {
        return WriteConfig(ordered, maxTimeMS, …);
    }
    WriteConfigBuilder& ordered(bool v)           { ordered = v; }
    WriteConfigBuilder& maxTimeMS(std::int32_t v) { maxTimeMS = v; }
    …
};
class WriteConfig {
    bool ordered = true;
    std::int32_t maxTimeMS = 0;
    …
public:
    // rule of zero
    WriteConfig& ordered(bool v)           { ordered = v; }
    WriteConfig& maxTimeMS(std::int32_t v) { maxTimeMS = v; }
    …
};
class WriteConfig {
    bool ordered; // default values not needed in this case
    std::int32_t maxTimeMS;
    …
public:
    // no friend declaration needed
    WriteConfig(bool ordered, std::int32_t maxTimeMS, …)
        : ordered(ordered)
        , maxTimeMS(maxTimeMS)
        , …
    {}
    …
};

class WriteConfigBuilder {
    bool m_ordered = true;
    std::int32_t m_maxTimeMS = 0;
    …
public:
    // rule of zero
    WriteConfig build() const {
        return WriteConfig(m_ordered, m_maxTimeMS, …);
    }
    WriteConfigBuilder& ordered(bool v)           { m_ordered = v; }
    WriteConfigBuilder& maxTimeMS(std::int32_t v) { m_maxTimeMS = v; }
    …
};
class WriteConfig {
    bool m_ordered = true;
    std::int32_t m_maxTimeMS = 0;
    …
public:
    // rule of zero
    WriteConfig& ordered(bool v)           { m_ordered = v; }
    WriteConfig& maxTimeMS(std::int32_t v) { m_maxTimeMS = v; }
    …
};
Source Link
G. Sliepen
  • 61.7k
  • 3
  • 61
  • 152

Remove support for big-endian systems

Unless you want your code to run natively on an IBM z/Architecture mainframe, there are basically no computers used anymore that run (let alone support) big-endian mode. I would recommend that you remove your code that supports big-endian machines, as it will not be used, thus rarely tested, and will only be a potential source of bugs and inefficiency.

Note that you are already assuming that bytes are 8 bits and that your code runs on machines that support 32-bit integers.

Don't make members mutable if not necessary

I don't see why the protected member variables of Op_MsgObj need to be mutable. You would only use that for some very specific use cases, like needing a mutex to guard access, or if you do some caching of expensive calculations.

In your case, remove mutable, then remove const from the operator>>() overload defined in Op_MsgObj.

class Connection can be replaced by a function

The class Connection just constructs a stream. That's it. It could be replaced by a single function:

SocketStream connectToDB(std::string_view host, int port,
                         std::string_view username,
                         std::string_view password,
                         std::string_view database,
                         ThorsAnvil::DB::Access::Options const& options) {
    SocketStream stream({host, port});
    …
    return stream;
}

Another option would be to make Connection actually be a stream itself, perhaps by just publicly inheriting from SocketStream. Or just fold the code into class DB.

Weird use of callback functions

Collection::insert() and Collection::del() both take an action parameter, which is a callback function that gets called with the response object. I don't know why it is designed like this. Either I would just return response from those functions so the caller can do what it wants with them without having to provide a callback function, or just remove it altogether if the caller is never going to do that.

There is a default callback declared for each overload, one of the overloads calls the other overload and moves the action along with it, wouldn't that create an infinite loop? It looks dodgy in any case. Why not just write:

WriteResponse insert(T const& doc) {
    return insert(doc, getWriteConfig());
}

WriteResponse insert(T const& doc, WriteConfig const& config) {
    db.getStream() << Op_Msg(Inserter<T>{*this, doc, config});
    WriteResponse response;
    db.getStream() >> Op_Msg(response);
    return response;
}

About the builder pattern

Using a builder interface is nice, but you are implementing it incorrectly. The goal of the builder is to construct an object with all the right parameters in one go. That constructor can then verify the combination of all those parameters is correct, and there will never be an object in a half-configured, potentially invalid state. So either fix this:

class WriteConfig {
    bool ordered; // default values not needed in this case
    std::int32_t maxTimeMS;
    …
public:
    // no friend declaration needed
    WriteConfig(bool ordered, std::int32_t maxTimeMS, …)
        : ordered(ordered)
        , maxTimeMS(maxTimeMS)
        , …
    {}
    …
};

class WriteConfigBuilder {
    bool ordered = true;
    std::int32_t maxTimeMS = 0;
    …
public:
    // rule of zero
    WriteConfig build() const {
        return WriteConfig(ordered, maxTimeMS, …);
    }
    WriteConfigBuilder& ordered(bool v)           { ordered = v; }
    WriteConfigBuilder& maxTimeMS(std::int32_t v) { maxTimeMS = v; }
    …
};

Or if you don't need that atomic construction, just move the setters from the builder directly into the type of object you want to create:

class WriteConfig {
    bool ordered = true;
    std::int32_t maxTimeMS = 0;
    …
public:
    // rule of zero
    WriteConfig& ordered(bool v)           { ordered = v; }
    WriteConfig& maxTimeMS(std::int32_t v) { maxTimeMS = v; }
    …
};

In the latter case, it even simplifies things a bit, in your example main() for example you'd then write:

collection.find<Person>(Query<AgeGreaterThan>{18}, ReadConfig{}.batchSize(5));

Lack of error checking?

I am seeing very little error checking in your code. The only hope is that all the stream operators called in your program throw exceptions on errors. The streams from the standard library do not however.