Skip to main content
7 of 7
added 1 character in body
G. Sliepen
  • 61.7k
  • 3
  • 61
  • 152

Simplify the design

Unless you plan to add many more storage options in the future, I would apply the KISS and YAGNI principles, and just create one single class that can write to both a string and a file descriptor, and which keeps track somehow of which ones to write to. Something like:

class DataStorage {
public:
    DataStorage(std::string &str, int fd = -1): str_{&str}, fd_{fd} {}
    DataStorage(int fd): fd_{fd} {}
    std::size_t Store(const char *buffer, std::size_t count);
    std::size_t GetTotalWritten() const { return total_count_; }
private:
    std::string *str_ = {};
    int fd = -1;
    std::size_t total_count_ = {};
};

Since -1 is guaranteed to be an invalid filedescriptor, and by storing the string as a pointer instead of a reference so it can be nullptr, we can check which of those to write to in Store():

std::size_t DataStorage::Store(const char *buffer, std::size_t count) {
    if (str_) {
        str_->append(buffer, count);
    }

    if (fd != -1) {
        ::write(fd_, buffer, count);
    }

    total_count_ += count;
    return count;
}

Use the correct types

You are using int everywhere you want a number, but there are different types of integers, and by not using the right ones you might introduce bugs (or even security issues) in your code. In particular, use std::size_t for most things that are sizes, counts and indices. It is guaranteed to be big enough to be able to address everything that fits in memory, whereas int is not.

Also be sure to store the return values from functions into a properly typed variable. In particular, write() returns a ssize_t. Note that you could also use auto to avoid having to specify the right type up front, for example like so:

auto ret = ::write(fd, buffer, count);

Missing error handling

You are not handling errors when writing to a file descriptor. ret can be negative for various reasons, some are recoverable (like if errno == EAGAIN), some are fatal. Also, write() might not write everything you want it to write, and can return a value that is less than count even if there is no error. If you want your code to be robust and not spuriously lose data you want to store, you have to handle all this, or alternatively return a proper error code to the caller so they can decide what to do.

Now you might think; I am returning something from Store() that can be used to check if there is an error, but unfortunately what you return is inconsistent between the different classes, and sometimes is not precise enough for the caller to see what went wrong. For example, DataStorageByFd::Store() returns the return value of write(), where a return value that is not the same as count means the caller has to do something about it, but DataStorageByStrAndFd::Store() just returns 0 or -1, but 0 is also returned if not all data was written to the file descriptor.

Ownership

Your data storage classes just store references to the file descriptor and the string. For the file descriptor, just store it by value; there is no point in storing it by reference. As for the string, it might be useful to store a reference, but it comes with a risk: what if the string you have a reference to goes out of scope before your data storage class? Consider storing the string by value in the class, and add a GetString() member function to get a reference to the string stored in your class.

Scalability

The approach you have taken does not scale well if you want to add other storage options. For just strings and file descriptors you needed four different classes, if you add one other storage option you need to add four more classes for all the combinations this adds. Basically, for \$N\$ storage methods you need \$2^N\$ classes, which is a maintainability nightmare. If you want this to be able to scale up, you need to find another way to do this, which brings me to the following:

Consider using C++'s file I/O

Instead of using POSIX functions to write to a file, consider using standard C++ functionality to write to files if that is possible. This will make your program more portable. For example:

class DataStorage {
public:
    DataStorage(std::ostream &os): os_(os) {}
    ...
private:
    std::ostream &os_;
};

std::size_t DataStorage::Store(const char *buffer, std::size_t count) {
    os_.write(buffer, count);
    ...
}

This is much more flexible, as now you can write to anything which is an ostream, which includes files, std::cout, and even std::stringstreams. If you want to support writing to multiple streams, you could consider storing the ostream references in a std::vector. Now, storing references in a container requires you to use std::reference_wrapper, and to allow you to pass an arbitrary number of references to the constructor of DataStorage you can use something like a std::initializer_list, which might look a little daunting if you haven't used those before, but it actually results in quite simple and concise code:

class DataStorage {
public:
    using ostream_ref = std::reference_wrapper<std::ostream>;

    DataStorage(std::initializer_list<ostream_ref> streams): streams_(streams) {}
    ...
private:
    std::vector<ostream_ref> streams_;
};

std::size_t DataStorage::Store(const char *buffer, std::size_t count) {
    for (auto& stream: streams_) {
        stream.get().write(buffer, count);
        ...
   }
   ...
}

With this, you can use it like so:

std::ofstream file("log.txt");
std::stringstream ss;

DataStorage ds({file, std::cout, ss});
ds.Store("Hello, world!\n", 14);

std::cout << "ss contains: " << ss.str();

Note that with this approach, you again only store references in the DataStorage class, making it store the concrete stream objects by value is more complicated, involving templates. It would look something like this:

template<typename... Ts>
class DataStorage {
public:
    DataStorage(Ts&&... streams): streams_(std::move(streams)...) {}

    void Store(const char *buffer, std::size_t count) {
        std::apply([=](Ts&... streams) {
            (streams.write(buffer, count), ...);
        }, streams_);
    }

    template<std::size_t I>
    auto& Get() {
        return std::get<I>(streams_);
    }

private:
    std::tuple<Ts...> streams_;
}

An issue here is that you cannot copy C++ IO streams, you can only take a reference or move them. The above will work when you pass in new objects:

DataStorage ds(std::ofstream("log.txt"), std::stringstream{});
ds.write("Hello, world!\n", 14);
std::cout << "ss contains: " << ds.Get<1>().str();

But you cannot do this:

DataStorage ds(std::cout, ...);

At least not without adding further complications to the code.

Going further

The above code works with all the stream types the standard library provides. But what if you need something not provided by the standard library, like writing to a POSIX file descriptor? In that case, you can implement your own custom stream class (see this question for some links). As long as it is or derives from std::ostream, it will then work with anything that can take a std::ostream.

It is also possible to turn your DataStorage class into something that inherits from std::ostream, so that you can do things like:

DataStorage ds(...);
ds << "Hello, world!\n";

And if the DataStorage class takes a list of std::ostreams in its constructor, but also is a std::ostream itself, you can write things like:

DataStorage ds1(...);
DataStorage ds2(...);
DataStorage ds({ds1, ds2});
G. Sliepen
  • 61.7k
  • 3
  • 61
  • 152