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.