10
\$\begingroup\$

I wrote a file manager API in C++ to make the read-write operations easier in read/write cases, needing big structured data to/from multiple files. I want to get a review of the library if it's readable and easy to understand, and if it serves its purpose.

File.hpp

#ifndef FILESAPI_FILE_H
#define FILESAPI_FILE_H

#include <iostream>
#include <mutex>
#include <fstream>
#include <vector>
#include <boost/assert.hpp>
#include "../utilities/DesignText.hpp"
#include "../utilities/Exceptions.hpp"

namespace FilesApi {

    /// Use in read/write for non-vectors overload operator. e.g: file >> rw_t<T>{val, 1};
    template<typename T> struct rw_s {
        T*          val;
        size_t      val_size;

        rw_s(T &value, size_t arr_size = 1) : val(&value), val_size(arr_size) {
            assert(arr_size > 0);

        }

        rw_s(T *value, size_t arr_size = 1) : val(value), val_size(arr_size) {
            assert(arr_size > 0);
            assert(value != nullptr);
        }
    };

    /// Wrapper function for creation of rw_t object, without need for specify the type after the function name
    /// Instead of call:    f << rw_t<int>(a, size);
    /// Call:               f << rw_soft(a, size);
    template<typename T> rw_s<T> rw_soft(T &value, size_t arr_size = 1) {
        return rw_s<T>(value, arr_size);
    }

    template<typename T> rw_s<T> rw_soft(T *value, size_t arr_size = 1) {
        return rw_soft(*value, arr_size);
    }

    /**
     * >> if file_mode is OPEN_IN_ACTION:
     * SINGLE_AND_DONE - read/write single time, and then close the file.
     * SINGLE_AND_MORE - read/write single time, but don't close the file yet. After single read/write the mode will automatic update to SINGLE_AND_DONE mode.
     * MULTIPLE - close the file only in programmer order, until then the file will be remain open.
     */
    enum class ReadWriteMode {
        SINGLE_AND_DONE,
        SINGLE_AND_MORE,
        MULTIPLE,
        DONE
    };

    enum class FileAction {
        READ,
        WRITE,
        NONE
    };

    enum class FileMode {
        ALWAYS_OPEN,
        OPEN_IN_ACTION
    };

    class File {
    private:
        bool is_ready;
        std::string name;
        std::string path;
        FileMode file_mode;
        ReadWriteMode read_write_mode;
        int multiple_times_left;
        FileAction file_action;
        std::mutex read_write_mutex;
        std::fstream file_ptr;
        bool is_open;
        std::ios_base::openmode read_flags;
        std::ios_base::openmode write_flags;
        bool use_exceptions;

        /**
         * Open file in specific format
         * \param mode_flags - fstream.open() flags.
         * \param new_file_action - Open purpose.
         */
        void open(std::ios_base::openmode mode_flags, const FileAction &new_file_action);

        /**
         * Auto update for the file's mode (READ / WRITE / NONE).
         */
        void update_rwm();

        /**
         * Close file
         * \param automatic - Close request from API(true) or from User(false)
         */
        void close(bool automatic);

        /**
         * Is file ready for read/write actions. Exception if file not ready.
         * \return Is file name not empty.
         */
        bool is_file_ready(int);

    public:
        /**
         * Ctor
         * \param file_name     - if @param file_path == "" => path/to/file/filename.bin else filename.bin
         * \param exceptions    - Throw exceptions on errors Or use bold cout messages.
         * \param file_path     - file's path.
         */
        explicit File(const std::string &file_name, bool exceptions = false, const std::string &file_path = "");

        /**
         * Close the file.
         */
        ~File();

        /**
         * Force close the file.
         */
        void close();

        /**
         * Set file's name.
         * \param new_name - New file's name.
         */
        void set_name(const std::string &new_name);

        /**
         * Set file's name.
         * \param new_name - New file's name.
         */
        File &operator=(const std::string &new_name);

        /**
         * Get file's name
         * \return File's name.
         */
        std::string get_name();

        /**
         * Is file ready for read/write actions. Without exception if file not ready.
         * \return Is file name not empty.
         */
        bool is_file_ready();

        /**
         * Init current file's mode
         * \param mode - How much reads/writes until the file will close.
         * \param multiple_times - if mode is multiple note how much times (-1 for unknown - won't close the file without programmer order/interrupt).
         */
        void init_read_write_mode(const ReadWriteMode &mode, int multiple_times = -1);

        /**
         * Init read fstream flags.
         * \param read_flags - When open function in read mode will occur, those flags will be in use.
         */
        void init_read_flags(std::ios_base::openmode read_flags = std::ios_base::in);

        /**
         * Init write fstream flags.
         * \param write_flags - When open function in write mode will occur, those flags will be in use.
         */
        void init_write_flags(std::ios_base::openmode write_flags = std::ios::out | std::ios::binary | std::ios::in);

        /**
         * Read to non-vector variable
         * \param T - variable type
         * \param val - variable address
         * \param data_size - in case of array- array's size.
         * \return this File object.
         */
        template<class T>
        File &read(T *val, size_t data_size = 1);

        /**
         * Read to vector variable
         * \tparam T - vector type
         * \param val - vector to read into (Have to be initialize with the size of inputs' count).
         * \param data_size - vector to read into (Have to be initialize with the size of inputs' count).
         * \return this File object.
         */
        template<class T>
        File &read(std::vector<T> &val);

        /**
         * Write non-vector variable
         * \tparam T - variable type
         * \param val - variable address
         * \param data_size - in case of array- array's size.
         * \return this File object.
         */
        template<class T>
        File &write(const T *val, size_t data_size = 1);

        /**
         * Write vector variable
         * \tparam T - vector type
         * \param val - vector to write.
         * \return this File object.
         */
        template<class T>
        File &write(const std::vector<T> &val);

        /**
         * Read to vector
         * \tparam T - vector type
         * \param data - vector to read into
         * \return this File object.
         */
        template<class T>
        File &operator>>(std::vector<T> &data);

        /**
         * Read to non-vector
         * \tparam T - variable type
         * \param info - {
         *                  val - variable non-vector to read into
         *                  val_size - in case of array- array's size (else leave as default 1)
         *               }
         * \return this File object
         */
        template<class T>
        File &operator>>(const rw_s<T> &info);

        /**
         * Write vector to file
         * \tparam T - vector type
         * \param data - vector to write
         * \return this File object
         */
        template<class T>
        File &operator<<(const std::vector<T> &data);

        /**
         * Write non-vector to file
         * \tparam T - variable type
         * \param info - {
         *                  val - variable non-vector to write
         *                  val_size - in case of array- array's size (else leave as default 1)
         *               }
         * \return this File object
         */
        template<class T>
        File &operator<<(const rw_s<T> &info);
    };

    template<class T>
    File &File::read(T *val, const size_t data_size) {
        if (!is_file_ready(0)) {
            return *this;
        }
        open(read_flags, FileAction::READ);
        std::lock_guard<std::mutex> guard(read_write_mutex);

        file_ptr.read((char *) (val), sizeof(T) * data_size);

        update_rwm();
        return *this;
    }

    template<class T>
    File &File::write(const T *val, const size_t data_size) {
        if (!is_file_ready(0)) {
            return *this;
        }
        open(write_flags, FileAction::WRITE);
        std::lock_guard<std::mutex> guard(read_write_mutex);

        file_ptr.write(reinterpret_cast<const char *>(val), sizeof(T) * data_size);

        update_rwm();
        return *this;
    }

    template<class T>
    File &File::read(std::vector<T> &val) {
        if (!is_file_ready(0)) {
            return *this;
        }
        open(read_flags, FileAction::READ);
        std::lock_guard<std::mutex> guard(read_write_mutex);

        file_ptr.read(reinterpret_cast<char *>(val.data()), sizeof(T) * val.size());

        update_rwm();
        return *this;
    }

    template<typename T>
    File &File::write(const std::vector<T> &val) {
        if (!is_file_ready(0)) {
            return *this;
        }
        open(write_flags, FileAction::WRITE);
        std::lock_guard<std::mutex> guard(read_write_mutex);

        file_ptr.write(reinterpret_cast<const char *>(val.data()), sizeof(T) * val.size());

        update_rwm();
        return *this;
    }

    template<class T>
    File &File::operator>>(std::vector<T> &data) {
        return read(data);
    }

    template<class T>
    File &File::operator>>(const rw_s<T> &info) {
        return read(info.val, info.val_size);
    }

    template<class T>
    File &File::operator<<(const std::vector<T> &data) {
        return write(data);
    }

    template<class T>
    File &File::operator<<(const rw_s<T> &info) {
        return write(info.val, info.val_size);
    }
}

#endif //FILESAPI_FILE_H

FilesManager.hpp

#ifndef FILESAPI_FILESMANAGER_H
#define FILESAPI_FILESMANAGER_H

#include <iostream>
#include <vector>
#include <map>
#include <memory>
#include "File.hpp"

namespace FilesApi {
    using add_data = std::tuple<const std::string, const std::string>;

    class FilesManager {
    private:
        std::map<std::string, std::shared_ptr<File>> files;
        size_t max_files; // zero for unlimited
        std::string files_path; // Leave "" if there is no single path for all of the files
        bool use_exceptions;

        void remove_unusable_files();

    public:
        /**
         * Ctor
         * \param exceptions - Throw exceptions on errors Or use bold cout messages.
         * \param max_files - Maximum files number to manage in this FilesManager object (0 for unlimited).
         * \param files_path - if @param files_path == "" => in new file associate you will have to supply full
         *                  file path, e.g: "path/to/file/filename.bin"
         *                  else supply only file name, e.g: "filename.bin", if @param files_path == "path/to/file/"
         */
        explicit FilesManager(bool exceptions = false, size_t max_files = 0, const std::string &files_path = "");

        /**
         * Add new file
         * \param id - file id (will be use to get this File object).
         * \param file - file's name or path (if @files_path == "").
         */
        void add(const std::string &id, const std::string &file);

        /**
         * Remove file
         * \param id - file's id
         */
        void remove(const std::string &id);

        /**
         * Get file
         * \param id - file's id
         */
        File &get(const std::string &id);

        /**
         * Add new file
         * \param data - tuple(0) => file id. tuple(1_ => file name or path (if files path is "").
         */
        void operator+=(const add_data &data);

        /**
         * Get file
         * \param id - file's id
         */
        File &operator[](const std::string &id);

        /**
         * Remove file
         * \param id - file's id
         */
        void operator-=(const std::string &id);
    };
}
#endif //FILESAPI_FILESMANAGER_H

File.cpp

#include "../headers/File.hpp"

namespace FilesApi {
    File::File(const std::string &file_name, bool exceptions, const std::string &file_path) {
        name = file_name;
        path = file_path;
        is_ready = !name.empty();
        use_exceptions = exceptions;
        is_open = false;
        file_mode = FileMode::OPEN_IN_ACTION;
        read_write_mode = ReadWriteMode::DONE;
        file_action = FileAction::NONE;
        write_flags = std::ios::out | std::ios::binary | std::ios::in;
        read_flags = std::ios_base::in;
    }

    File::~File() {
        std::lock_guard<std::mutex> guard(read_write_mutex);
        close();
    }

    void File::open(std::ios_base::openmode mode_flags, const FileAction &new_file_action) {
        std::lock_guard<std::mutex> guard(read_write_mutex);
        if (!is_file_ready(0)) {
            if (is_open) {
                file_ptr.close();
                is_open = false;
            }
            return;
        }
        if (file_action != new_file_action) {
            file_ptr.close();
            is_open = false;
            if (file_action != FileAction::NONE) {
                std::cout
                        << DesignText::make_colored("Pay attention: file mission replaced by another one. (file closed)",
                                                    DesignText::Color::RED, false) << std::endl;
            }
        }
        file_action = new_file_action;

        if (!is_open) {
            file_ptr.open(path + name, mode_flags);
            if (file_ptr.fail()) {
                is_open = false;
                if (!use_exceptions) {
                    std::cout << DesignText::make_colored("Error Opening file: " + path + name,
                                                        DesignText::Color::RED, true) << std::endl;
                } else {
                    throw FileOpenException(path + name);
                }
            }
            is_open = true;
            std::cout << DesignText::make_colored("File has safely opened.", DesignText::Color::GREEN, false)
                      << std::endl;
        }
    }

    void File::close(bool automatic) {
        if ((!automatic) || (file_mode == FileMode::OPEN_IN_ACTION)) {
            if (is_open) {
                file_ptr.close();
                is_open = false;
                read_write_mode = ReadWriteMode::DONE;
                file_action = FileAction::NONE;
                std::cout << DesignText::make_colored("File has safely closed.", DesignText::Color::GREEN, false)
                          << std::endl;
            }
        }
    }

    void File::close() {
        close(false);
    }

    void File::update_rwm() {
        if (!is_file_ready(0)) {
            return;
        }
        switch (read_write_mode) {
            case ReadWriteMode::SINGLE_AND_DONE:
            case ReadWriteMode::DONE:
                close(true);
                break;
            case ReadWriteMode::SINGLE_AND_MORE:
                read_write_mode = ReadWriteMode::SINGLE_AND_DONE;
                break;
            case ReadWriteMode::MULTIPLE:
                if (multiple_times_left > -1 && !--multiple_times_left) {
                    multiple_times_left = -1;
                    close(true);
                }
                break;
        }
    }

    void File::init_read_write_mode(const ReadWriteMode &new_mode, const int multiple_times) {
        read_write_mode = new_mode;
        multiple_times_left = multiple_times;
    }

    void File::init_read_flags(const std::ios_base::openmode new_read_flags) {
        read_flags = new_read_flags;
    }

    void File::init_write_flags(const std::ios_base::openmode new_write_flags) {
        write_flags = new_write_flags;
    }

    void File::set_name(const std::string &new_name) {
        if (!new_name.empty()) {
            name = new_name;
            is_ready = true;
            return;
        }
        if (name.empty()) {
            is_ready = false;
        }
    }

    std::string File::get_name() {
        return name;
    }

    bool File::is_file_ready(int) {
        if (!is_ready) {

            if (!use_exceptions) {
                std::cout << DesignText::make_colored("Pay attention: file name is empty. can't open this file.",
                                                    DesignText::Color::RED, true) << std::endl;
            } else {
                throw FileNotReadyException();
            }
            return false;
        }
        return true;
    }

    bool File::is_file_ready() {
        return is_ready;
    }

    File &File::operator=(const std::string &new_name) {
        set_name(new_name);
        return *this;
    }
}

FilesManager.cpp

#include "../headers/FilesManager.hpp"

namespace FilesApi {
    FilesManager::FilesManager(bool exceptions, size_t max_files, const std::string &files_path)
            : max_files(max_files), files_path(files_path), use_exceptions(exceptions) {
    }

    void FilesManager::add(const std::string &id, const std::string &file) {
        remove_unusable_files();
        if (max_files == 0 || files.size() + 1 < max_files) {
            files.insert(std::pair<std::string,
                    std::shared_ptr<File>>(id, std::make_shared<File>(file, use_exceptions, files_path)));
        }
    }

    void FilesManager::remove(const std::string &id) {
        remove_unusable_files();
        files.erase(id);
    }

    File &FilesManager::get(const std::string &id) {
        remove_unusable_files();
        File *ret_file = files[id].get();
        if (ret_file == nullptr) {
            files[id] = std::make_shared<File>("", use_exceptions, files_path);
            ret_file = files[id].get();
        }
        return *ret_file;
    }

    void FilesManager::operator+=(const add_data &data) {
        add(std::get<0>(data), std::get<1>(data));
    }

    File &FilesManager::operator[](const std::string &id) {
        return get(id);
    }

    void FilesManager::operator-=(const std::string &id) {
        remove(id);
    }

    void FilesManager::remove_unusable_files() {
        for (auto &file : files) {
            if (file.second && !file.second->is_file_ready()) {
                files.erase(file.first);
            }
        }
    }
}

Use example:

#include <iostream>
#include <vector>
#include <complex>
#include "../src/headers/FilesManager.hpp"

using namespace std;
using namespace FilesApi;

int mainFilesManagerOperatorsTest() {
    FilesManager fm(false, 0, "../TestFiles/");
    string files[] = {"test_file.bin", "test_file2.bin"};

    fm["1"] = files[0];

    vector<complex<float>> wdata = {{1, 9}, {3, 75}, {213.34, 21.4}, {153.1, 15.85}};
    vector<complex<float>> rdata(wdata.size());

    fm["1"].init_read_write_mode(ReadWriteMode::SINGLE_AND_DONE);
    //fm.get("1").write(wdata.data(), wdata.size()); // Use it as complex<float> array. Possible.
    fm["1"].write(wdata);
    fm["1"].init_read_write_mode(ReadWriteMode::SINGLE_AND_DONE);
    fm["1"].read(rdata);

    fm += add_data("5", files[1]); // Add file to collection
    int a = 12;
    int b;
    fm["5"] << rw_soft(a); // Work
    fm["5"].write(&a); // Work
    fm["5"] >> rw_soft(b); // Work
    cout << b << endl; // Prints 12

    fm -= "5"; // Remove the file from collection
    fm["5"] << rw_soft(a); // Error
    fm["5"].write(&a); // Error
    fm["5"] >> rw_soft(b); // Error

    //fm["2"] = files[1];
    fm += add_data("2", files[1]);

    for (size_t i = 0; i < rdata.size(); i++) {
        cout << rdata[i] << endl;
    }


    fm["2"].init_read_write_mode(ReadWriteMode::MULTIPLE);
    for (size_t i = 0; i < 100; i++) {
        fm["2"].write(&i);
    }

    //f.init_read_write_mode(ReadWriteMode::MULTIPLE);

    size_t j;
    for (size_t i = 0; i < 100; i++) {
        fm["2"].read(&j);
        cout << j << " ";
    }
    cout << endl;

    return 0;
}

You can find more examples/code utilities on GitHub.

\$\endgroup\$
  • 3
    \$\begingroup\$ Welcome to Code Review! Yes, you got the right place, helping people improve their code is exactly what we do here. Hope you get some great answers! \$\endgroup\$ – Phrancis Jun 22 '19 at 0:11
  • \$\begingroup\$ Can you describe in a few sentences what you need the "file descriptors" for? I don't see an immediate value in repeatedly writing f["1"] when a simple variable would serve the same purpose? \$\endgroup\$ – Roland Illig Jun 22 '19 at 9:02
  • \$\begingroup\$ @RolandIllig if I understand what do you mean, I don't want to hold 200 variables if I need to manage i/o operations over 200 different files. It's true that a simple variable would serve the same purpose, but it is easier to mange some items with the same access generic way. A little priciple to compare to: An abstract classes. there it's with different objects types, here it with different files. \$\endgroup\$ – KorelK Jun 22 '19 at 9:56
3
+50
\$\begingroup\$

File class:

rw_s(T &value, size_t arr_size = 1);
...
template<typename T> rw_s<T> rw_soft(T &value, size_t arr_size = 1);
...
File &read(T *val, size_t data_size = 1);

The default size argument is likely to cause problems. I'd suggest separating the interface for reading / writing arrays from single values.

  • Reading / writing a single value can be done by taking a reference argument and no size argument.
  • Reading and writing an array should take both parameters, and not have a default size (because the user will always want to specify it).

They can still use the same underlying implementation.


template<class T>
File &File::operator >> (std::vector<T> &data) {
    return read(data);
}

These operators are unnecessary duplication of the read and write functions.

Note that the C++ standard streams use operator>> and operator<< for formatted (text) input and output, whereas your file class only does binary input and output. This may cause confusion.

std::vector is only one type of container, and users are likely to require support for data structures or their own custom classes. It might be better to implement the stream operators as free functions rather than member functions. That would allow users to specify their own custom versions. All these implementations can then use the read() and write() member functions (or other stream operators).


    file_ptr.read((char *)(val), sizeof(T) * data_size); // note: missing reinterpret_cast?
    file_ptr.read(reinterpret_cast<char *>(val.data()), sizeof(T) * val.size());
    file_ptr.read(reinterpret_cast<char *>(val.data()), sizeof(T) * val.size());
    file_ptr.write(reinterpret_cast<const char *>(val), sizeof(T) * data_size);

T must be a trivially copyable type for these to work. I suggest adding a static_assert(std::is_trivially_copyable_v<T>, ..."");


template<class T>
File &File::read(std::vector<T> &val) {
    if (!is_file_ready(0)) {
        return *this;
    }
    open(read_flags, FileAction::READ);
    std::lock_guard<std::mutex> guard(read_write_mutex);

    file_ptr.read(reinterpret_cast<char *>(val.data()), sizeof(T) * val.size());

    update_rwm();
    return *this;
}

template<typename T>
File &File::write(const std::vector<T> &val) {
    if (!is_file_ready(0)) {
        return *this;
    }
    open(write_flags, FileAction::WRITE);
    std::lock_guard<std::mutex> guard(read_write_mutex);

    file_ptr.write(reinterpret_cast<const char *>(val.data()), sizeof(T) * val.size());

    update_rwm();
    return *this;
}

One often uses a dynamic container like std::vector when one does not know the required size in advance. It would be more helpful to store the size of the vector too, instead of forcing users to deal with this themselves.


    template<class T>
    File &read(T *val, size_t data_size = 1);

Note that even basic types are different sizes on different platforms, and may be big or little endian. This may not matter to you right now, but it does make your read and write implementations inherently dangerous.

A safer interface would ensure that the user specifies the size of the type they wish to write, and convert to a specific endianness before writing (and do the opposite for reading).


File::File(const std::string &file_name, bool exceptions, const std::string &file_path) {
    name = file_name;
    path = file_path;
    is_ready = !name.empty();
    use_exceptions = exceptions;
    is_open = false;
    file_mode = FileMode::OPEN_IN_ACTION;
    read_write_mode = ReadWriteMode::DONE;
    file_action = FileAction::NONE;
    write_flags = std::ios::out | std::ios::binary | std::ios::in;
    read_flags = std::ios_base::in;
}

Prefer to use a constructor initializer-list, instead of initializing members in the body of the constructor, so that initialization only happens once:

File::File(const std::string &file_name, bool exceptions, const std::string &file_path):
    is_ready(!file_name.empty()),
    name(file_name),
    path(file_path),
    ...

is_ready and is_open are unnecessary duplication. We could instead write them as functions that return !name.empty() and file_ptr.is_open().


File::~File() {
    std::lock_guard<std::mutex> guard(read_write_mutex);
    close();
}

Shouldn't the guard be inside the close function, since the user can call close themselves?


    std::string get_name();
    bool is_file_ready();
    // ... and others

Member functions that don't change member data must be const.


    bool is_file_ready(int);

Use a function with a different name (e.g. throw_if_not_ready), instead of an unused int parameter.


    void init_read_write_mode(const ReadWriteMode &mode, int multiple_times = -1);

This function does not really do any "initialization". Perhaps set_... would be better.


void File::set_name(const std::string &new_name)

It's extremely inconsistent and confusing to allow the user to change this while a file is open! The same issue exists with several other functions in the class.


File &File::operator=(const std::string &new_name) {
    set_name(new_name);
    return *this;
}

A file name string is not logically a file. This is unnecessary duplication of set_name.


    void init_write_flags(std::ios_base::openmode write_flags = std::ios::out | std::ios::binary | std::ios::in);

Why in for writing?


void File::open(std::ios_base::openmode mode_flags, const FileAction &new_file_action)

FileAction should probably be determined by checking the flags. As it is, we have duplicate information that could be inconsistent.


FilesManager class:

std::map<std::string, std::shared_ptr<File>> files;

The file ownership is not shared with anything, so we have no reason to use a shared_ptr.


    std::string files_path;

It turns out we are setting the path member of File to this same value for every single file! That's a huge amount of unnecessary duplication.


void FilesManager::operator+=(const add_data &data) {
    add(std::get<0>(data), std::get<1>(data));
}

File &FilesManager::operator[](const std::string &id) {
    return get(id);
}

void FilesManager::operator-=(const std::string &id) {
    remove(id);
}

We're duplicating code here, and also adding complexity with the add_data struct. Generally in C++ we should avoid overloading operators, unless it's for very common and unambiguous mathematical operations.

We would expect const versions of get() and operator[]. The behavior of adding a missing file is very surprising for a get() function.


void FilesManager::remove_unusable_files()

This function seems to be called whenever we access or do anything with the files. It would be simpler to just avoid storing any unusable files in the first place.


void FilesManager::add(const std::string &id, const std::string &file)
File &FilesManager::get(const std::string &id)
void FilesManager::remove(const std::string &id)

Do we not call these from different threads? If the File class needs to be thread-safe, doesn't the FilesManager too?


void FilesManager::add(const std::string &id, const std::string &file)

It seems the user has to know the file name to add it to the FilesManager. In that case, they already have the means to get the filename for a given id, and there is probably no need to store the filename in the File.


Purpose:

Overall it's unclear what the purpose of any of this code is. It seems a mixture of the following:

  • Binary IO - this is useful, but the current implementation isn't platform independent, and has other flaws.
  • Thread-safe reading and writing - but being thread-safe in itself doesn't really help with anything, and the FileManager isn't thread-safe.
  • Associating file paths with ids - but we can do that more clearly with a separate map.
  • Some sort of automatic file opening and closing system - but we can do that better with simple RAII (std::fstream file handles close the file when the handle goes out of scope - the user can simply hold the handle for as long as they need).

Writing binary data:

Utilities for writing binary data are definitely helpful. But we can implement these around the existing std::ostream and std::istream through a simpler interface:

enum class Endian { Big, Little };

void WriteBinary(std::ostream& stream, Endian endian, bool value);
void WriteBinary(std::ostream& stream, Endian endian, char value);
void WriteBinary(std::ostream& stream, Endian endian, signed char value);
void WriteBinary(std::ostream& stream, Endian endian, unsigned char value);
// ... (same for other pod types)

void ReadBinary(std::istream& stream, Endian endian, bool& value);
// ... (similar for reading)

Note that correct platform independence is quite difficult. Internally these functions would need to:

  • Decide on a fixed number of bits to write for each type (and handle issues with types being different sizes on different platforms).
  • Use std::memcpy to convert to the unsigned type of the corresponding fixed size.
  • Change the byte order from system endianness to the output endianness (if needed).
  • Finally call stream.write();

We (or users of the library) can extend this by defining similar functions for custom types:

template<class T>
void WriteBinary(std::ostream& stream, Endian endian, std::vector<T> const& value);

template<class KeyT, class ValueT, class PredicateT, class AllocatorT>
void WriteBinary(std::ostream& stream, Endian endian, std::std::map<KeyT, ValueT, PredicateT, AllocatorT> const& value);
\$\endgroup\$
  • \$\begingroup\$ Thanks! It'll take some time for me to work on those improvements those days, but it absolutely help me to understand how to create better libraries in the future and gave me some additional points to focus whenever I want to build new libraries (and this is what I looked for). \$\endgroup\$ – KorelK Sep 7 '19 at 13:30
3
\$\begingroup\$

Don't reimplement functionality that is already provided by the standard library

Why do you need a FilesManager? It is just a glorified std::map. The only extra features it has is a limit for the maximum number of files, and that you can specify a base directory. In my opinion, that's not enough to warrant this class.

Why would you ever want to set a limit to the number of files? That will only cause it to fail to add a file when you want to. Also, the FilesManager::add() function will just return without any error when it didn't insert a File into files.

What's even weirder is that the FilesManager::get() function will actually add a new File to files when it couldn't find the id, bypassing the max_files restriction. But unless you remember to call File::set_name(), it will try to open the base path...

Many functions will never return any errors, and they don't do any error checking themselves. For example, when you close a file, you don't check if the failbit is set.

So, I suggest you avoid making wrappers for std::fstream and std::map, and just let the application use those standard library classes directly.

Focus on making the new functionality generic enough to work on std::fstream

The main novelty of your code is reading and writing vectors and arrays from/to files in an easy way. You can make your templates that do this work on file streams instead. For example, to ensure you can write vectors to an output stream, you can write:

template<class T>
std::ostream &operator<<(std::ostream &out, const std::vector<T> &data) {
    out.write(reinterpret_cast<const char *>(val.data()), sizeof(T) * val.size());
    return *this;
}

Adding locks to read() and write() is not useful.

If you are going to read and write to a file from multiple threads, the mutex you use to synchronize calls to File::read() and File::write() are not going to be of much help, because these mutexes don't guarantee anything about the order in which those functions are going to be executed. So, any thread will probably want to do its own high level locking, to ensure for example that multiple consecutive reads in one thread actually also read consecutive data from the file.

Maps are not thread-safe

While you added locks to individual functions of class File, you did not add a mutex to class FilesManager to protect against concurrent access of the map files. This means that if multiple threads try to add or remove files from a FilesManager object, it will corrupt the map, at best resulting in a crash, at worst resulting in a security hole.

Example without class FilesManager and class File

The following code shows how your use example would work by only providing the operator overloads to read and write vectors to files, the rw_soft() wrapper. It uses the standard library for everything else. The code looks mostly the same, and has roughly the same level of verbosity.

#include <complex>
#include <iostream>
#include <fstream>
#include <map>
#include <vector>

template<typename T>
struct rw_s {
    T *data;
    size_t size;
};

template<typename T> rw_s<T> rw_soft(T &data, size_t size = 1) {
    return rw_s<T>{&data, size};
}

template<typename T>
std::ostream &operator<<(std::ostream &out, const std::vector<T> &vec) {
    out.write(reinterpret_cast<const char *>(vec.data()), sizeof(T) * vec.size());
    return out;
}


template<typename T>
std::istream &operator>>(std::istream &in, std::vector<T> &vec) {
    in.read(reinterpret_cast<char *>(vec.data()), sizeof(T) * vec.size());
    return in;
}

template<typename T>
std::ostream &operator<<(std::ostream &out, const rw_s<T> &info) {
    out.write(reinterpret_cast<const char *>(info.data), sizeof(T) * info.size);
    return out;
}

template<typename T>
std::istream &operator>>(std::istream &in, const rw_s<T> &info) {
    in.read(reinterpret_cast<char *>(info.data), sizeof(T) * info.size);
    return in;
}

int main() {
    const auto create = std::ios_base::in | std::ios_base::out | std::ios_base::trunc;

    std::map<const std::string, std::fstream> fm;
    fm["1"] = std::fstream("test_file.bin", create);

    std::vector<std::complex<float>> wdata = {{1, 9}, {3, 75}, {213.34, 21.4}, {153.1, 15.85}};
    std::vector<std::complex<float>> rdata(wdata.size());

    fm["1"] << wdata;
    fm["1"].seekg(0);
    fm["1"] >> rdata;

    fm["5"] = std::fstream("test_file2.bin", create);
    int a = 12;
    int b = 0;
    fm["5"] << rw_soft(a);
    fm["5"].seekg(0);
    fm["5"] >> rw_soft(b);
    std::cout << b << '\n';

    fm.erase("5");
    fm["5"] << rw_soft(a); // Error
    fm["5"].seekg(0); // Error
    fm["5"] >> rw_soft(b); // Error

    for (auto &&val: rdata) {
        std::cout << val << '\n';
    }

    fm["2"] = std::fstream("test_file2.bin", create);

    for (size_t i = 0; i < 100; i++) {
        fm["2"] << rw_soft(i);
    }

    fm["2"].seekg(0);

    for (size_t i = 0; i < 100; i++) {
        size_t j = 0;
        fm["2"] >> rw_soft(j);
        std::cout << j << ' ';
    }
    std::cout << '\n';
}

Avoid using maps to store a collection of objects if it's not necessary.

This is not so much about the implementation of your classes, but rather about your intended use of class FilesManager. Maps are not free; looking up an element in a map means traversing a tree structure, comparing strings at every node in this tree. While you mention you don't want to hold 200 files in 200 separate variables, there are many other ways to avoid having to declare 200 variables, some of them simpler and/or faster than a std::map. For example, you could just make an array or a std::vector of 200 files.

\$\endgroup\$
  • \$\begingroup\$ I do agree with you that there are things to improve, but you missed up some of the functionality in my implementation. I thank you for the issues you mentioned, however I want to make a point about the choosing in map instead of a vector. In large projects, you want to keep the code as readable and understandable for the largest period as you can. After 2-3 years, you will have hard time in understanding, what your previous one in the job, ment when he wrote files[12], instead of files["available_users.bin"]. \$\endgroup\$ – KorelK Sep 6 '19 at 23:07
  • \$\begingroup\$ One important thing that I set as a target in my implementation, is the easy way of use in the functionality. Instead of making hard rows in every using part like: const auto create = std::ios_base::in | std::ios_base::out | std::ios_base::trunc; std::map<const std::string, std::fstream> fm; fm["1"] = std::fstream("test_file.bin", create);, give even to a starter programmer in this language the ability to use this code, and to fully understand the use way. \$\endgroup\$ – KorelK Sep 6 '19 at 23:15
  • \$\begingroup\$ @KorelK If you are literally going to write files["available_users.bin"] in the code, why not write std::fstream available_users_file("available_users.bin") to begin with? I can only see a map being useful if the code doesn't know these names up front and really needs to map a file object to some string identifier. \$\endgroup\$ – G. Sliepen Sep 7 '19 at 8:15
  • \$\begingroup\$ @KorelK I agree that the const auto create = ... is a bit verbose and not user-friendly. However, you would normally use either std::ifstream or std::ofstream to read and write files, and get the expected behavior without having to explicitly give std::ios_base flags. And, it's always better for a starter programmer to learn how to use the standard library properly. Here I used std::fstream to match your example code where you reuse a File for both writing and reading. \$\endgroup\$ – G. Sliepen Sep 7 '19 at 8:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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