Read a .ini file

Question

I got frustrated with C++ not being Python, so I put together a .ini file reader for myself. The last C++ code I posted, the feedback was that it's better to use free functions that private static methods when possible, so I tried to apply that here. I also tried to follow the single layer of abstraction principle as dogmatically as possible to see what would happen.

A questionable decision I made was to have, for example, isSectionHeader assume that isComment has already been called. I'm starting to think that was a mistake, and that I should have just done

bool isSectionHeader(const std::string& line)
{
    if (isComment(line))
    {
         return false;
    }
    etc.
}

so feedback in those areas would be nice. Also, because I do not get much chance to explore past C++98, if you see any places where modern language features would help, I would love to hear about it.

Handling whitespace is deliberately ommitted.

ConfigParser.h

#pragma once
#include <map>
#include <string>
#include <vector>

typedef std::map<std::string, std::string> Section;
typedef std::map<std::string, Section> Config;

class ConfigParser
{
public:
    ConfigParser(const std::string& pathname);
    Section getSection(const std::string& sectionName) const
    {
        return mSections.at(sectionName);
    }
    std::string get(const std::string& sectionName, const std::string& key) const
    {
        return mSections.at(sectionName).at(key);
    }
    std::vector<std::string> getSections() const;

private:
    Config parseFile(const std::string& pathname);
    void parseLine(const std::string& line, Config& sections);
    void addSection(const std::string& line, Config& sections);
    void addKeyValuePair(const std::string& line, Config& sections) const;

    std::string mCurrentSection;
    const Config mSections;
};

ConfigParser.cpp

#include "ConfigParser.h"
#include <algorithm>
#include <cctype>
#include <stdexcept>
#include <fstream>
#include <string>
#include <sstream>
#include <vector>


// The function in this namespace will go into a custom string util library
namespace stringUtil
{
    std::vector<std::string> split(const std::string& string, char delimiter = ' ')
    {
        std::vector<std::string> result;
        std::stringstream stream(string);
        std::string word;
        while (std::getline(stream, word, delimiter))
        {
            result.push_back(word);
        }
        return result;
    }
}

namespace
{
    bool isComment(const std::string& line)
    {
        return line[0] == ';';
    }

    std::string extractSectionName(const std::string& line)
    {
        return std::string(line.begin() + 1, line.end() - 1);
    }

    bool isSectionHeading(const std::string& line)
    {
        if (line[0] != '[' || line[line.size() - 1] != ']')
        {
            return false;
        }
        const std::string sectionName = extractSectionName(line);
        return std::all_of(sectionName.begin(), sectionName.end(), std::isalpha);
    }

    bool isKeyValuePair(const std::string& line)
    {
        // Assume we have already checked if it's a comment or section header.
        return std::count(line.begin(), line.end(), '=') == 1;
    }

    void ensureSectionIsUnique(const std::string& sectionName, const Config& sections)
    {
        if (sections.count(sectionName) != 0)
        {
            throw std::runtime_error(sectionName + " appears twice in config file");
        }
    }

    void ensureKeyIsUnique(const std::string& key, const Section& section)
    {
        if (section.count(key) != 0)
        {
            throw std::runtime_error(key + " appears twice in section");
        }
    }

    void ensureCurrentSection(const std::string& line, const std::string& currentSection)
    {
        if (currentSection.empty())
        {
            throw std::runtime_error(line + " does not occur within a section");
        }
    }

    std::pair<std::string, std::string> parseKeyValuePair(const std::string& line)
    {
        std::vector<std::string> pair = stringUtil::split(line, '=');
        return std::pair <std::string, std::string>(pair[0], pair[1]);
    }
}

ConfigParser::ConfigParser(const std::string& pathname) :
    mCurrentSection(""),
    mSections(parseFile(pathname))
{
}

std::vector<std::string> ConfigParser::getSections() const
{
    std::vector<std::string> sectionNames;
    for (auto it = mSections.begin(); it != mSections.end(); ++it)
    {
        sectionNames.push_back(it->first);
    }
    return sectionNames;
}

Config ConfigParser::parseFile(const std::string& pathname)
{
    Config sections;
    std::ifstream input(pathname);
    std::string line;
    while (std::getline(input, line))
    {
        parseLine(line, sections);
    }
    return sections;
}

void ConfigParser::parseLine(const std::string& line, Config& sections)
{
    if (isComment(line))
    {
        return;
    }
    else if (isSectionHeading(line))
    {
        addSection(line, sections);
    }
    else if (isKeyValuePair(line))
    {
        addKeyValuePair(line, sections);
    }
}

void ConfigParser::addSection(const std::string& line, Config& sections)
{
    const std::string sectionName = extractSectionName(line); 
    ensureSectionIsUnique(sectionName, sections);
    sections.insert(std::pair<std::string, Section>(sectionName, Section()));
    mCurrentSection = sectionName;
}

void ConfigParser::addKeyValuePair(const std::string& line, Config& sections) const
{
    ensureCurrentSection(line, mCurrentSection);
    const auto keyValuePair = parseKeyValuePair(line);
    ensureKeyIsUnique(keyValuePair.first, sections.at(mCurrentSection));
    sections.at(mCurrentSection).insert(keyValuePair);
}

Answer 1

Style is always in the eyes of the beholder

For a lot of cases you will get differing answers, for the problem at hand, parsing a file to produce an instance of a class representing the information there are a lot of different ways some coming down to -taste-. To help deciding between some of them it's always good to look at how it will be used. This review is somewhat affected by how I have done similar things in the past, take it as guidelines and points to think about rather than a categorical 'this is the way it should be done'.

Missing Unit tests

Unit tests not only help actually testing your functionality, they will also guide you to a better api by exercising your existing api.

Exceptions in the constructor

The way you wrote your constructor makes it hard to use the configparser class, the constructor might throw but there is no accessor to the config data that was parsed with the filename in the constructor. Which means you force the user to do all the processing in the try/catch clause. But if you consider an ill-formed .ini file a unlikely condition that should abort execution (that is a valid point of view), this means you want to see a failure as soon as possible this is a valid pattern.

ConfigParser parser("./config.ini"); // Should be correct, abort when not
// Use parser here ... 

Otherwise this pattern is necessary

try {
    ConfigParser parser("./config.ini");
    // Use parser here ...
}
catch (std::exception e) {
    // Handle errors
}
// Can't get to parser here ... it's outside of the scope 
// of the try/catch and there is no parser.getConfig() call
// to copy the config in the try part

Possible overuse of exceptions

There are a lot of exceptions that might be thrown from your code, ones that you generate and others that are cause by some of the functions that you call e.g. std::map.at() will except on a key that doesn't exist. Considering how you have to deal with exceptions or what happens when an exception occurs should all of these really be exceptions or just failures or even just return empty results. That is dependent on what you are trying to achieve. For example the wikipedia page for .ini talks about variables outside of the scope of a section, yet you decide to throw an exception when that occurs. I can't say whether it's right or wrong to do that. you need to look at how you, or someone else might use your code and decide whats right or wrong.

Catch failures of library calls

Goes a little bit hand in hand with the previous case, you're not catching any of the possible errors of any of the containers and library calls that you make. This will probably produce error messages that might be hard to debug. If you actually do let the program fail report errors in terms of the operation of the program like you do when you create the custom exceptions.

Wrong abstraction

You access the configuration through the parser object. I'd like to see access to the configuration through the configuration object. By just using typedef you expose the implementation of parser, none of the public functions in the parser have to do with parsing they are all about accessing the configuration, the should be on the configuration object

Assumptions are ok

With regard to the note that you posted at the beginning of your post, it is ok to make assumptions, this is code private to the class, if the order that you determine is

• check for comment
• check for section
• check for variable

Then it is perfectly ok to assume in the following checks that you already checked for a comment. Commenting your code to reflect this is ok. If you have other requirements later on then refactor and improve.

Prefer struct over std::pair

There are no benefits to using std::pair in your case a struct will be much more readable. .first and .second don't mean anything to the reader of your code. .variableName .value actually do mean something

Beware of the whitespace

Handwritten files contain whitespaces in unexpected places, it might be easier for you to just deal with those rather than relying for things to be at the beginning or at the end of lines. e.g. isComment(), isSectionHeading()

Passing a member variable

I am not too hot on what you are doing in some of the ConfigParser functions, passing Config& sections which is defacto a member variable of the parser, you might as well operate on the member itself rather than passing it all the time, you use mCurrentSection without passing it ... if you wrote the parser all by using free functions (which you could) then this would be appropriate.

Alternative Interface

So this is how I might do this, but remember this is independent of your requirements. The parser would use the public interface to write to the config data, using the same api you could set defaults in the code, in cases where there might not be a config file. The parse is started via a free function, the user doesn't even need to see the ConfigParser class. The parser still throws exceptions on read errors, the apropriate level would be up to you. The Config interface lets the parser decide wether to be permissive with duplicated pieces or disallow them. I.e. use hasXXX and throw, or just overwrite, or overwrite and warn rather than fail. You could use the empty section as a catchall for values outside of sections.

Config parseConfigFile(const std::string& filename, const Config& default = Config())
{
    Config result(default);
    try {
        Configparser parser(filename, result);
        result = parser.getConfig();
    }
    catch (std::exception e)
    {
        // Handle errors according to specs
    }
    return result;
}

Config {

    // Sets a variable, will create a section, will create the variable, will overwrite the 
    // current value of the variable
    bool setValue(const std::string& section, const std::string& name, const std::string& value);

    // return true if variable exists
    bool hasValue(const std::string& section, const std::string& name);

    // return value of variable, returns "" if the variable doesn't exist
    std::string getValue(const std::string& section, const std::string& name);

    // alternative... 
    // return false if variable doesn't exist, otherwise return true and write to value
    bool tryGetValue(const std::string& section, const std::string& name, std::string* value);

    // return true if section exists
    bool hasSection(const std::string& section);

    // return list of sections in the configuration
    std::vector<std::string> getSections();
}

class ConfigParser {
public:
    ConfigParser(const std::string& filename, const Config& defaults = Config());
    Config getConfig();
}