Simple .cfg parser

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

ConfigReader.hpp

#include 
#include 

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 records;
};

ConfigReader.cpp

```
#include
#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 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 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 & 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 + valu

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.

StackExchange Code Review Q#28179, answer score: 3