Minimalist config file reader/writer

Question

I have written a simple configuration file reader and writer in D that is intended to parse string[string] associative arrays (the whole file is one array) and everything will either be a string or null. I would love any suggestions to help shorten or optimize the code.

import std.stdio: File;
import std.string: indexOf, strip, stripRight, split, startsWith;
import std.range: enumerate;
import std.algorithm: remove;

/// Read a simple configuration in the format 'key = value' from `std.stdio.File`.
string[string] readConfig(File configFile) {
    configFile.rewind();

    string[string] configData;

    foreach (text; configFile.byLine()) {
        text = text.strip();

        if (text.length == 0 || text[0] == '#')
            continue;

        const ptrdiff_t commentIndex = text.indexOf('#');
        const ptrdiff_t assignIndex = text.indexOf('=');

        if (commentIndex > -1)
            text = text[0 .. commentIndex];

        const string key = text[0 .. assignIndex].idup.stripRight();
        string value = text[assignIndex + 1 .. $].idup.strip();

        if (value == "null")
            value = null;

        configData[key] = value;
    }

    return configData;
}

// From path
string[string] readConfig(const string configPath) {
    File configFile = File(configPath);
    const string[string] configData = configFile.readConfig();
    configFile.close();
    return configData;
}

/// Write a string[string] as configuration data to a file.
void writeConfig(File configFile, const string[string] configData) {
    configFile.rewind();

    string[] configKeys = configData.keys();
    string textBuffer;

    foreach (text; configFile.byLine()) {
        const char[] stripped = text.strip();

        const long varIndex = stripped.startsWithAny(configKeys);

        if (varIndex > -1) {
            textBuffer ~= configKeys[varIndex] ~ " = " ~ (configData[configKeys[varIndex]] || "null") ~ '\n';
            configKeys = configKeys.remove(varIndex);
        } else
            textBuffer ~= text ~ '\n';
    }

    foreach (varName; configKeys) {
        textBuffer ~= varName ~ " = " ~ (configData[varName] || "null") ~ '\n';
    }

    configFile.truncate(0);
    configFile.write(textBuffer);
}

// From path
void writeConfig(const string configPath, const string[string] configData) {
    File configFile = File(configPath, "w+");
    configFile.writeConfig(configData);
    configFile.close();
}

/// Find and replace a variable value in a configuration file.
void setVariable(File configFile, const string varName, const string varValue) {
    configFile.rewind();

    string textBuffer;
    bool varFound;

    foreach (text; configFile.byLine()) {
        const char[] stripped = text.strip();

        if (stripped.startsWith(varName)) {
            textBuffer ~= varName ~ " = " ~ (varValue || "null") ~ '\n';
            varFound = true;
        } else
            textBuffer ~= text ~ '\n';
    }

    if (!varFound)
        textBuffer ~= varName ~ " = " ~ (varValue || "null") ~ '\n';

    configFile.truncate(0);
    configFile.write(textBuffer);
}

/// From path
void setVariable(const string configPath, const string varName, const string varValue) {
    File configFile = File(configPath, "w+");
    configFile.setVariable(varName, varValue);
    configFile.close();
}

/// Cross-platform function truncate an `std.stdio.File` at an offset position in bytes.
void truncate(File file, long offset) {
    version (Windows) {
        import core.sys.windows.windows: SetEndOfFile;

        file.seek(offset);
        SetEndOfFile(file.windowsHandle());
    } else version (Posix) {
        import core.sys.posix.unistd: ftruncate;

        ftruncate(file.fileno(), offset);
    } else
        static assert(0, "truncate() is not implimented for thos OS version.");
}

private long startsWithAny(const char[] searchString, const char[][] compareStrings) {
    foreach (index, compare; compareStrings.enumerate())
        if (searchString.startsWith(compare))
            return index;

    return -1;
}

An example configuration file can be seen at https://github.com/spikespaz/windows-tiledwm/blob/master/hotkeys.conf.

Answer 1

string value = text[assignIndex + 1 .. $].idup.strip();

Since you're using stripRight on the line above, why not stripLeft here? Also, instead of doing idup.strip, use strip.idup - that way you aren't allocating spaces that you then strip off and ignore, wasting memory.

configFile.close();

Unnecessary - File will auto-close when leaving scope.

void writeConfig(File configFile, const string[string] configData) {
    configFile.rewind();

    string[] configKeys = configData.keys();
    string textBuffer;

Consider using Appender!string instead of string here. As the name indicates, it's optimized for appending, while string is more a jack-of-all-trades.

Next up:

(configData[configKeys[varIndex]] || "null")

|| always returns a boolean result, so this will always evaluate to true, or 0x01, so every single line you write will be corrupted. You can use this utility function:

auto or(T...)(T args) {
    foreach (e; args)
        if (e)
            return e;
    assert(0);
}

Usage:

configData[configKeys[varIndex]].or("null")

All-in-all, writeConfig looks a bit curious. I assume it's written the way it is in order to keep comments, and perhaps as an attempt at optimization. However, it's using startsWithAny on every line, giving it a N^2 complexity. This can be improved by looking up the prefix in the AA:

const assignIndex = stripped.indexOf("=");
const prefix = stripped[0..assignIndex == -1 : 0 : assignIndex];
auto var = prefix in configData;

if (var) {
    textBuffer ~= prefix ~ " = " ~ ((*var).or("null")) ~ '\n';
    configKeys = configKeys.remove(varIndex);
} else
    textBuffer ~= text ~ '\n';

Another problem with using startsWithAny is that a variable called foo will overwrite one called foobar, if they happen to be arranged that way in the AA keys. Your solution also discards comments on the form var=value #comment.

Next:

File configFile = File(configPath, "w+");

You're trying to read lines from this file. Use "r+", not "w+".

The problems with writeConfig are generally also present in setVariable. setVariable is also somewhat confusing - when would you set the value of a setting, but not update the string[string]?

Lastly, have you even tested this code? As pointed out, it will write corrupted data to file, it will discard comments, it will overwrite variables with similar names, and it will ignore file contents.

I would suggest in the future you write at least some kind of unit tests to check that the code does what you want it to. Something along the lines of:

unittest {
    import std.array : array;
    import std.algorithm.iteration : map;
    enum filename = "config.conf";
    // Create initial config file
    {
        auto f = File(filename, "w");
        f.writeln("#comment");
        f.writeln("var2 = foo");
        f.writeln("var1 = bar");
    }

    // Read contents and check that correct data is read
    auto cfg = readConfig(filename);
    assert(cfg.length == 2);
    assert("var2" in cfg);
    assert(cfg["var2"] == "foo");
    assert("var1" in cfg);
    assert(cfg["var1"] == "bar");

    // Write back and check that correct data is written
    writeConfig(filename, cfg);
    auto contents = File(filename).byLine.map!idup.array;
    // Trailing \r included in byLine is bug #11465, and a windows-only problem.
    assert(contents[0] == "#comment\r");
    assert(contents[1] == "var2 = foo\r");
    assert(contents[2] == "var1 = bar\r");
}

Now, I can't in good conscience write only negative stuff, and there are certainly good things in your implementation. Your naming is great - names are explanatory, not too short and not too long. Comments are used when they are useful, not too many, and not too few. Functions are of good length, and the control flow is easy to follow, and utility functions are introduced where sensible. Imports are tidy, and selective imports are used to give quick information about which function comes from where. All in all, it's a pleasant read, marred by implementation problems.

If I were to write an implementation of the functionality you have, I would also encapsulate it in a struct or class:

struct Config {
    string[string] _variables;
    this(string filename)
    {
        // read into _variables
    }

    string opIndex(string name) {
        return _variables[name];
    }

    void opIndexAssign(string value, string name) {
        _variables[name] = value;
    }

    void save() {
        // save to file
    }
}

This way, I have all the functionality inside a nice little box that I can pass around, and I'm free to keep other information like comments in the data structure, without exposing that complexity to the user of the code.