C++ Read File line by line

Question

I wanted to have a good review on my code I wrote today to read files in C++ line by line. The file can look something like this:

# Program configuration

network.port=30000 # The port the server uses
network.ip=127.0.0.1 # The IP the server uses

Notes:

1. Lines that start with # won't be processed
2. Lines that are empty won't be processed
3. All the data before the # on a line will be processed
4. The data will be split by =, before the = the key and after it the value

configuration.h:

#pragma once
#include <fstream>
#include <map>
#include <sstream>
class configuration
{
public:
    configuration(const char* path);
    ~configuration();
    std::string get_value(const char* key);
private:
    std::map<std::string, std::string> values;
    std::ifstream file;
};

configuration.cpp:

#include "configuration.h"
#include <Windows.h>
#include <iostream>
#include "logger.h"

configuration::configuration(const char* path)
{
    try
    {
        file.open(std::string(path), std::ios::in);
        values = std::map<std::string, std::string>();

        if (file.is_open())
        {
            std::string line;
            while (std::getline(file, line))
            {
                if (line.find("#") != 0 && !line.empty() && line.find("="))
                {
                    std::string part = line.substr(0, line.find("#"));
                    std::string key = part.substr(0, line.find("="));
                    std::string value = part.substr(line.find("=") + 1);
                    values.insert_or_assign(key, value);
                }
            }
        }
        else
        {
            logger::log_error("File %s is not open!", path);
        }
    }
    catch (const std::ios_base::failure& ex)
    {
        logger::log_error(ex.code().message().data());
    }
}

std::string configuration::get_value(const char* key)
{
    return values[key];
}


configuration::~configuration()
{
    file.close();
}

Usage would be:

configuration* config = new configuration("file.cnf");
config->get_value("network.port");

Answer 1

Regarding the API:

• consider using std::string const & arguments instead of char const*. They are less error prone.

• your configuration uses a std::ifstream input that is only used in the constructor; the file should not be a member of the configuration.

• the code to read the configuration from a stream should (probably) be put into an std::istream& operator>>(std::istream&, configuration&);. This would:

  • integrate well with existing APIs on streams
  • abstract away the concrete type of the stream (your configuration would be able to read a string in memory - using a std::istringstream just as easily as a file on the disk)
  • separate concerns
  • allow you to test the configuration class in separation

Consider this public API instead:

class configuration final
{
public:
    using storage_type = std::map<std::string,std::string>;

    /// \b instantiate from set of values (that can be empty)
    explicit configuration(storage_type values = {}) noexcept;

    ///. \note default destructor is enough to have - so don't declare it here

    /// \return a const reference instead of an object
    ///
    /// we are not modifying the class so we can be const and noexcept
    const std::string&
    get_value(const std::string& key) const noexcept
    {
        using namespace std::string_literals;
        try
        {
            return values.at(key);
        }
        catch(std::out_of_bounds const&)
        {
            static const auto empty = ""_s;
            return empty;
        }
    }

private:
    storage_type data_;
};

Regarding the implementation (general notes):

try
{
    file.open(std::string(path), std::ios::in);

• consider instantiating a file with a file name (opening directly from the construction instead of explicitly calling open)

  if (file.is_open())
  

• this is bad: the file could be open, but the stream could be in an invalid state; when you want to check on a stream's state for reading, simply rely on it's boolean conversion (use if(file) instead).

• this could be a simple assignment: values.insert_or_assign(key, value);: values[key] = value;

• logging messages in case of errors is very very bad:

  • it adds a dependency to a logging module (every time you want to reuse the class somewhere else, you will also have to add the logging dependencies)

  • it does too much: when you want to reuse the code you will want to do that for reading a configuration, not for logging. Logging should not be in here at all.

  • it gives you no possibility for checking for an error in client code, unless your logging framework has extra functionality to give you the last of it's error messages and you want to parse those (and all of a sudden you are solving the x-y problem)

  • you should either catch the errors here (like you already do) and throw a higher abstraction exception instead, or simply remove the try-catch block and allow client code to catch exceptions - or not. One of the best features of exceptions is that you can choose not to handle anything at the current level and you have to write no explicit code for that.

Final reading code for this:

std::istream& operator>>(std::istream& in, configuration& c)
{
    auto storage = storage_type{};
    auto line = std::string{};
    while(getline(in, line))
    {
        if(line.empty())
            continue;
        auto split = line.find_first_of("#=");
        if (split == std::string::npos)
            throw std::runtime_error{ "bad data format" };

        // strip comment
        if(line[split] == '#')
            line = line.substr(0, split);
        ... 
    }

    c = configuration{ std::move(storage) };
    return in;
}

Edit: here's some client code, that is possible due to the separation of the parsing and the construction:

#include <sstream>
#include <fstream>

// test code
std::istringstream test_input{ R"(
# Program configuration

network.port=30000 # The port the server uses
network.ip=127.0.0.1 # The IP the server uses)"};

auto c1 = configuration{};
assert(test_input >> c1);

// useful code:
std::ifstream in{ "file.cnf" };
auto c2 = configuration{};
if!(in >> c2)
    std::cerr << "Error reading file.cnf\n";