Simple .cfg parser

Question

I'm currently working to implement a simple configuration file parser. I intend to add more functional down the road.

ConfigReader.hpp

#include <string>
#include <vector>

class ConfigReader
{
    public:
        struct record
        {
            std::string section;
            std::string name;
            std::string value;
        };

        ConfigReader();

        explicit ConfigReader (const std::string & file);

        bool readfile (const std::string & file);

        std::string get_string (const std::string & tsection, 
        const std::string & tname, std::string tdefault = std::string());

private:
    std::vector<record> records;
};

ConfigReader.cpp

#include <fstream>
#include "ConfigReader.hpp"

namespace
{
/* Erases leading tabs, leading spaces; trailing tabs, trailing spaces. */
std::string & trim (std::string & str)
{
    // leading tabs / spaces
    int i = 0;

    while (i < (int) str.length() && (str[i] == ' ' || str[i] == '\t'))
        ++i;

    // erase leading tabs / spaces
    if (i > 0)
        str.erase (0, i);

    int j = i = str.length();

    while (i > 0 && (str[i - 1] == ' ' || str[i - 1] == '\t'))
        --i;

    // erase trailing tabs / spaces
    if (i < j)
        str.erase (i, j);

    return str;
}

/* Erases tabs and spaces between the variable's name and its value. */
std::string & normalize (std::string & str)
{
    // Erases leading tabs, leading spaces; trailing tabs, trailing spaces.
    trim (str);

    // i is the start of the section of tabs and spaces.
    // j is the end of said section.
    std::size_t i, j;
    i = j = 0;

    while (i < str.length())
    {
        if (str[i] == ' ' || str[i] == '\t')
        {
            j = i + 1;

            // find the end of section of tabs and spaces.
            while (j < str.length() && (str[j] == ' ' || str[j] == '\t'))
                ++j;

            // if the section consists of just one character,
            // then erase just one character.
            // otherwise, remove j - i characters.
            if (j == i)
                str.erase (i, 1);
            else
                str.erase (i, j - i);
        }
        else
        {
            str[i] = std::tolower (str[i]);
            ++i;
        }
    }

    return str;
}

/* Check if a line consists only of spaces and tabs */
bool spaceonly (const std::string & line)
{
    for (int i = 0, j = line.length(); i < j; ++i)
    {
        if (line[i] != ' ' && line[i] != '\t')
            return false;
    }

    return true;
}

/* Check if a line is valid */
bool isvalid (std::string & line)
{
    normalize (line);
    std::size_t i = 0;

    // if the line is a section
    if (line[i] == '[')
    {
        // find where the section's name ends
        std::size_t j = line.find_last_of (']');

        // if the ']' character wasn't found, then the line is invalid.
        if (j == std::string::npos)
            return false;

        // if the distance between '[' and ']' is equal to one,
        // then there are no characters between section brackets -> invalid line.
        if (j - i == 1)
            return false;
    }

    /* Check if a line is a comment */
    else if (line[i] == ';' || line[i] == '#' || (line[i] == '/' && line[i + 1] == '/'))
        return false;

    /* Check if a line is ill-formed */
    else if (line[i] == '=' || line[i] == ']')
        return false;

    else
    {
        std::size_t j = line.find_last_of ('=');

        if (j == std::string::npos)
            return false;

        if (j + 1 >= line.length())
            return false;
    }

    return true;
}

// parse the line and write the content to our vector
void parse (std::vector<ConfigReader::record> & records, std::string & section, std::string & line)
{
    std::size_t i = 0;

    // if the line is a section
    if (line[i] == '[')
    {
        ++i;
        std::size_t j = line.find_last_of (']') - 1;

        section = line.substr (i, j);
    }
    // if the line is a variable + value
    else
    {
        ConfigReader::record temp;
        temp.section = section;

        // construct the name of the variable
        std::size_t j = line.find ('=');
        std::string name = line.substr (i, j);
        temp.name = name;

        // construct the variable's value
        std::size_t k = line.find ('=') + 1;
        std::size_t z = line.find (';');
        z = (z == std::string::npos) ? line.length() : z;
        std::string value = line.substr (k, z); // bug ? if the line is width = 32; then the semicolon is not erased.
        temp.value = value;

        records.push_back (temp);
    }
    }
}

bool ConfigReader::readfile (const std::string & file)
{
    records.clear();

    std::ifstream config (file);

    if (!config.is_open())
        return false;

    std::string section;
    std::string buffer;

    std::size_t i = 0;

    while (std::getline (config, buffer, '\n'))
    {
        if (!spaceonly (buffer))
        {
            if (isvalid (buffer))
                parse (records, section, buffer);
            else{}
            //                std::cout << "Failed at line " << i;
        }

        ++i;
    }

    return true;
}

std::string ConfigReader::get_string (const std::string & tsection,
                                  const std::string & tname, std::string tdefault)
{
    for (std::size_t i = 0; i < records.size(); ++i)
    {
        if (records[i].section == tsection && records[i].name == tname)
        {
            return records[i].value;
        }
    }

    record temp;
    temp.section = tsection;
    temp.name = tname;
    temp.value = tdefault;
    records.push_back (temp);

    return tdefault;
}

ConfigReader::ConfigReader (const std::string & file) : records()
{
    readfile (file);
}

ConfigReader::ConfigReader() : records()
{

}

The syntax of the config file is simple:

[video]
width = 1920;
etc;

What should I change? Improve? Are there any errors? (Well, there is one in parse.)

Answer 1

Use standard algorithms where applicable. For instance, trim could be rewritten:

std::string& trim(std::string& s) {
  auto is_whitespace = [] (char c) -> bool { return c == ' ' || c == '\t'; };
  auto first_non_whitespace = std::find_if_not(begin(s), end(s), is_whitespace);
  s.erase(begin(s), first_non_whitespace);
  auto last_non_whitespace = std::find_if_not(s.rbegin(), s.rend(), is_whitespace)
      .base();
  s.erase(std::next(last_non_whitespace), end(s));
  return s;
}

Likewise, normalize could be written like this:

std::string& normalize(std::string& s) {
    s.erase(std::remove_if(begin(s), end(s),
                           [] (char c) { return c == ' ' || c == '\t'; }),
            end(s));
    std::transform(begin(s), end(s), begin(s),
                   [] (char c) { return std::tolower(c); });
    return s;
}

Note that the intent of this code is quite clear -- I didn't need to add comments to let you know what each section of the code was meant to be doing (although it is necessary to know standard C++ algorithms and idioms like erase-remove). It took me a few minutes of careful reading to realize that you were removing all whitespace in your normalize function; my version makes this very clear. Are you sure you want to remove all whitespace? Couldn't some future configuration use string types?

PS I chose to use C++11 features like auto and lambda, but this code can be written in C++03 with not very much more boilerplate code.

---

In isvalid, you declare std::size_t i = 0. This is confusing for a couple of reasons. When I see a variable declared non-const, I expect that it will change. Even if it were const, i is not meaningful here. Use 0 instead of i throughout that function to make it more readable. Likewise, your use of j is not self-documenting. I expect i and j to be loop variables. For j it's a bit more forgivable because it has such a narrow scope, but last_bracket is a name that tells me what the variable is actually for.

Also, throughout isvalid, you dereference line[0] and line[1], but line might be empty after normalization. Even if you think that's not the case because of spaceonly guarding it, you should add an assertion that documents the precondition. As it stands, your code will access out-of-bounds memory when the line contains only whitespace and a single '/'.

---

get_string has funny semantics. For one thing, if my config file is

[Video]
Foo = Bar

I will get an empty string when calling getline("Video", "Foo") because you normalize the configuration file but not the queries.

Why do you store a default value if the query fails to find a record? This is not behavior I would expect; in fact, I'd expect get_string to be declared const. If you insist on storing a temp value, you should add a constructor for record and rewrite the storage as records.push_back(record(tsection, tname, tdefault));; in modern compilers, this will result in no copy being made while your version requires a copy.

You should also consider a different data structure. There are a few reasonable options.

• Keep using a vector, but sort it and use lower_bound to do lookups.
• Use a map keyed on section or section and name.
• Use an unordered_map.

It depends on future class features, the expected size of config files, and the frequency of insertions, but the vector approach is almost certainly best -- see this blog entry for example.

---

This list is not exhaustive, but it should get you started.