Skip to main content
2 of 7
added 61 characters 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.

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");

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 would be much more complicated, involving templates.

G. Sliepen
  • 61.7k
  • 3
  • 61
  • 152