Reading configuration from a file for video rendering

Question

I haven't done anything similar so I got nothing to go on. The point was to load everything from the file and I thought a struct would be a good choice to be the handler to store the information.

The loading part is, however, not really something that is to my satisfaction as I find it very primitive and illusive. I try to walk around issues as I don't really know how to handle them if anything goes awry.

    //Set Config Value from File
bool ConfigValue(std::string contents, std::string setting, unsigned int& MySettings){
    int pos1 = NULL, pos2 = NULL;
    pos1 = contents.find(setting);
    pos2 = contents.find('\n', pos1);
    if (pos1 == -1)
        return false;
    else
        pos1 += setting.length();
    contents = contents.substr(pos1, pos2 - pos1);
    for (int i = 0; i < contents.length();i++){
        if (!(contents[i] >= '0' && contents[i] <= '9' || contents[i] == ' ')) 
            return false;
    }
    MySettings = std::stoi(contents);
    return true;
}
//Reads ConfigFile with some safe measures if it's wrongly written for some reason
Config ConfigFile(){
start:
    video::E_DRIVER_TYPE renderer = video::EDT_DIRECT3D9;
    //Set Default if file doesn't contain them.
    Config MySettings;
    std::string line;
    fstream input("config.ini", fstream::in);


    if (input){
        //Read config file to buffer
        std::stringstream buffer;
        buffer << input.rdbuf();
        std::string contents(buffer.str());

        if (!ConfigValue(contents, "width=", MySettings.Width) || //Set Width
            !ConfigValue(contents, "height=", MySettings.Height) || //Set Height
            !ConfigValue(contents, "fullscreen=", MySettings.Fullscreen) || //Set Fullscreen
            !ConfigValue(contents, "renderer=", MySettings.renderer))  //Gets renderer value
            goto makeDefault;

        //Set renderer according to value obtained
        switch (MySettings.renderer){
        case 0: MySettings.renderer = video::EDT_DIRECT3D8;
            break;
        case 1: MySettings.renderer = video::EDT_DIRECT3D9;
            break;
        case 2: MySettings.renderer = video::EDT_OPENGL;
            break;
        default: goto makeDefault;
        }
    }
    else{
    makeDefault:
        input.close();
        input.open("config.ini", fstream::out);
        input << "//renderer options - 0: D3D8, 1: D3D9, 2: OpenGL\nwidth=640\nheight=480\nfullscreen=0\nrenderer=1"; //Set Default Resolution;
        goto start;
    }

    return MySettings;
}

Config:

struct Config{
    unsigned int renderer;
    unsigned int Width;
    unsigned int Height;
    unsigned int Fullscreen; //should be bool

};

As you can see, I use goto a lot as a kind of safety net. If anything ever goes awry (in my detection), I reset everything. However, the tests I do to make sure it's correct is something I would like reviewed.

The config in its current state is supposed to contain this:

//renderer options - 0: D3D8, 1: D3D9, 2: OpenGL
width=640
height=480
fullscreen=0
renderer=1

The first line is just an instruction for the renderer as it's kinda hard to guess it. Nothing else needs any instructions so they are normal. It's always supposed to be "WhatItIs=Value". (Worth noting is that Fullscreen is a bool, as 0 and 1 works the same I see no reason to mess with conversion).

Any suggestion on this is wanted as I really find pretty much all of this code to be extremely bad even though it works. But as I got nothing to compare to it, I can only guess.

Answer 1

I see a number of things that could help you improve your code.

Make sure you include all required headers

The necessary headers are not included in the question which makes it a bit harder to review than it needs to be. The standard ones that are required appear to be these:

#include <iostream>
#include <string>
#include <fstream>
#include <sstream>

Pass file names as parameters

Rather than hardcoding a filename twice (leading to the potential error of not having them match!), pass it as a parameter instead. This reduces the hidden linkages in the code and makes it easier to see what's happening.

Be careful with signed versus unsigned

The code currently contains this:

for (int i = 0; i < contents.length();i++){
    if (!(contents[i] >= '0' && contents[i] <= '9' || contents[i] == ' ')) 
        return false;
}
MySettings = std::stoi(contents);

However, on some platforms, size_t is unsigned, so the loop is comparing an unsigned and a signed number. I'd avoid that problem by declaring i to be of type size_t.

Use nullptr rather than NULL

Modern C++ uses nullptr rather than NULL. See this answer for why and how it's useful. Once you do that, you'll see that you have been abusing NULL and using it as an integer value instead of just a pointer. For example, the code includes this:

int pos1 = NULL, pos2 = NULL;

In that context, both of those should be 0 because the variables are not pointers.

Use namespaces when required

The std namespace is used in some locations within the code, but not all. In particular, in this line:

fstream input("config.ini", fstream::in);

Needed to be converted to this to compile on my machine:

std::fstream input("config.ini", std::ios::in);

Create a default constructor

In the case there is no config file, you'd like to have some default settings. The better way to do that is to have a default constructor for Config. I'd write it like this:

Config() : renderer{1}, Width{640}, Height{480}, Fullscreen{0} {}

Create a stream inserter

At some point, you will undoubtedly want to write the configuration file. To do that, I'd recommend writing a stream inserter like this:

friend std::ostream& operator<<(std::ostream& out, const Config& cfg) {
    return out << "//renderer options - 0: D3D8, 1: D3D9, 2: OpenGL"
        "\nwidth=" << cfg.Width << 
        "\nheight=" << cfg.Height << 
        "\nfullscreen=" << (cfg.Fullscreen ? 1 :0 ) << 
        "\nrenderer=" << cfg.renderer << '\n';
}

Eliminate goto

The above two suggestions allow you to eliminate the start label and the associate goto by writing the else clause like this:

else{
    std::ofstream out(filename);
    out << MySettings;
}

Use standard algorithms to simplify

The code contains ConfigValue which returns a bool indicating whether the configuration was updated. However, a more generic way to handle this might instead be to write a function to take a line and extract a name, value pair. Here's how I'd write such a function:

std::pair<std::string, int> value(std::string line) {
    std::pair<std::string, int> val;
    // remove any comments
    auto n = line.find("//");
    if (n != std::string::npos) {
        line.erase(n);
    n = line.find('=');
    if (n != std::string::npos) {
        line[n] = ' ';  // replace '=' with space
        std::stringstream ss(line);
        ss >> val.first >> val.second;
    }
    return val;
}

Write a stream extractor

In the same spirit as writing the stream inserter, we can write a stream extractor. Using the value function above as a member function of Config, it could be this:

friend std::istream& operator>>(std::istream& in, Config& cfg) {
    Config temp;
    std::string line;
    int fields = 0xF;
    for (std::string line; fields && std::getline(in, line); ) {
        auto pair=temp.value(line);
        if (pair.first == "width") {
            temp.Width = pair.second;
            fields &= ~0x01;
        } else if (pair.first == "height") {
            temp.Height = pair.second; 
            fields &= ~0x02;
        } else if (pair.first == "renderer") {
            temp.renderer = pair.second; 
            fields &= ~0x04;
        } else if (pair.first == "fullscreen") {
            temp.Fullscreen = pair.second; 
            fields &= ~0x08;
        }
    }
    if (fields == 0) {
        std::swap(temp, cfg);
    } else {
        in.setstate(std::ios::failbit);
    }
    return in;
}

Note that unlike the current code, this code only updates the default if all four values were successfully read. If you don't want that, you can just omit all references to fields in this function.

Make the object take care of itself

Since all of this code is associated with the Config struct, why not just actually make it a C++ class and have it take care of its own initialization? For that, we could replace the ConfigFile function with a constructor:

Config(const std::string& filename)
: renderer{1}, Width{640}, Height{480}, Fullscreen{0} 
{
     std::ifstream input(filename);
    input >> *this;
    if (!input) {
        std::ofstream out(filename);
        out << *this;
    }
}

Test function

I created a sample main to drive this code. It looks like this:

int main()
{
    Config cfg("configreader.test");
    std::cout << cfg;
}

Naturally, you'll want to add more error and range checking but I hope this inspires you to rework your code into something more object-oriented.

Answer 2

int pos1 = NULL, pos2 = NULL;
pos1 = contents.find(setting);
pos2 = contents.find('\n', pos1);
if (pos1 == -1)
    return false;
else
    pos1 += setting.length();

If you return a value, the function execution stops, so there's no need to use an else, you can just make a guard clause that returns upon the condition. There's also no need to set pos2 if you're not going to use it, so you ought to move it down a bit...

int pos1 = NULL, pos2 = NULL;
pos1 = contents.find(setting);
if (pos1 == -1){
    return false;
}

pos2 = contents.find('\n', pos1);
pos1 += setting.length();