3
\$\begingroup\$

This program converts a proprietary text data format to the conf or ini file format. I have some questions on the code:

  1. Is the high level code design coherent and easy to read?
  2. Does the code use 'good code design practice'?
  3. How could this code be improved?

The input format is a filename to describe the 'subject' of the key value pairs in the file. The filename is mapped to a [section] in the ini file format.

The input files contain key value pairs separated by a colon.

generate.sh can be used to create the input files.

generate.sh:

#!/bin/bash

if [ $# -ne 1 ]; then
    echo "Usage: generate <directory>"
    exit 1
fi

directory=$1

lastchar=${directory: -1}

if [ $lastchar != '/' ]; then
    directory="${directory}/"
fi

printf "SPORT : AX3\nNEWS : Sky7\nCOMEDY : Art5\n" > ${directory}channels.txt
printf "ADDRESS : 192.168.4.33\nPORT : 5088\n" > ${directory}server.txt

Implementation code (main.cpp):

#include <iostream>
#include <string>
#include <filesystem>
#include <fstream>
#include <sstream>
#include <unordered_map>
#include <vector>

class config {
public:
    using keyvalues = std::unordered_map<std::string, std::string>;

    config() {}

    config(std::istream& istrm) {
        parse(istrm);
    }

    config(const std::string& contents) {
        std::stringstream strm(contents);
        parse(strm);
    }

    config(config& cfg) = default;

    void set(const std::string& section, const std::string& key, const std::string& value) {
        conf[section][key] = value;
    }
    bool remove(const std::string& section, const std::string& key) {
        auto sectionitems = conf.find(section);
        // if section not found, return fail to remove
        if (sectionitems == conf.end()) {
            return false;
        }

        // if key not found, return fail to remove
        auto keyValue = sectionitems->second.find(key);
        if (keyValue == sectionitems->second.end()) {
            return false;
        }

        return sectionitems->second.erase(keyValue) != sectionitems->second.end();
    }

    using iterator = std::unordered_map<std::string, keyvalues>::iterator;
    iterator begin() { return conf.begin(); }
    iterator end() { return conf.end(); }

    const std::string serialise() {
        std::string serialised;
        for (auto& section : conf) {
            serialised += '[' + section.first + "]\n";
            for (auto keyvalue : section.second) {
                serialised += keyvalue.first + '=' + keyvalue.second + '\n';
            }
        }
        return serialised;
    }

private:
    std::unordered_map<std::string, keyvalues> conf;

    void parse(std::istream& strm) {
        std::string line;
        std::string sectionname;
        while (std::getline(strm, line, '\n')) {
            if (!line.empty()) {
                if (line.size() > 2 && line[0] == '[' && line[line.size() - 1] == ']') {
                    sectionname = line.substr(1, line.size() - 2);
                }
                else {
                    // get key and value for this section
                    const std::string key = line.substr(0, line.find('='));
                    const std::string value = line.substr(line.find('=') + 1);
                    conf[sectionname][key] = value;
                }
            }
        }
    }
};


std::ostream& operator<<(std::ostream& strm, config& cfg) {
    for (auto& section : cfg) {
        strm << "section name: " << section.first << '\n';
        for (auto& keyvalue : section.second) {
            strm << "key: " << keyvalue.first << ", value: " << keyvalue.second << '\n';
        }
    }
    return strm;
}

class FileHandler {
public:
    static std::vector<std::string> GetFilenames(const std::string& directoryName) {
        std::vector<std::string> files;
        const std::filesystem::path folder{ directoryName };
        for (auto const& dir_entry : std::filesystem::directory_iterator{folder})
        {
            files.push_back(dir_entry.path().string());
        }
        return files;
    }

    static const std::string GetFilenamePart(const std::string& fullpath) {
        const std::filesystem::path filepath { fullpath };
        return filepath.stem().string();
    }

    // generate file and return full path to generated file
    static const std::string GenerateFilename(const std::string& directory, const std::string& filename) {
        return directory.back() != SEPARATOR ? directory + SEPARATOR + filename : directory + filename;
    }

private:
#ifdef _WIN32
    static const char SEPARATOR{ '\\' };
#else
    static const char SEPARATOR{ '/' };
#endif
};

class StringTransformer {
public:
    static const std::pair<std::string, std::string> SplitOn(const std::string& s, const char delim) {
        size_t pos = s.find(delim);
        if (pos != std::string::npos) {
            return { s.substr(0, pos), s.substr(pos + 1) };
        }
        else {
            return { "","" };
        }
    }
};


int main(int argc, char* argv[]) {

    if (argc < 2 || argc > 3) {
        std::cout << "Usage: prop2ini <directory> [config filename]\n";
        return 1;
    }

    const std::string directory = argv[1];
    const std::string filename = argc == 3 ? argv[2] : "config.conf";

    const std::string fullpath = FileHandler::GenerateFilename(directory, filename);


    // get files in directory
    std::vector<std::string> files = FileHandler::GetFilenames(directory);

    config cfg;

    for (auto file : files) {
        const std::string section = FileHandler::GetFilenamePart(file);

        std::ifstream inputFile(file);

        std::string line;
        while (std::getline(inputFile, line)) {
            auto keyValue = StringTransformer::SplitOn(line, ':');
            cfg.set(section, keyValue.first, keyValue.second);
        }
    }

    const std::string serialised = cfg.serialise();

    std::ofstream outputFile(fullpath, std::ios::app);

    outputFile << serialised;

    // informational message
    std::cout << "proprietary configuration files in " << directory << " copied to " << filename << '\n';
}
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Simplify generate.sh

You should not have to worry about the trailing slash in the directory name if you just add one unconditionally:

printf "SPORT : AX3\nNEWS : Sky7\nCOMEDY : Art5\n" > $1/channels.txt

The above will always do what you want, as two or more consecutive slashes just count as one. Another option to avoid the issue would be to just do:

cd $1
printf "SPORT : AX3\nNEWS : Sky7\nCOMEDY : Art5\n" > channels.txt

However, that is a bit unsafe: what if the directory you pass doesn't exist? The cd would then fail, but the subsequent command would then cause channels.txt to be written to the current directory. A good way to avoid this is to tell the shell to abort on any errors, by setting the -e flag as soon as possible:

#!/bin/sh
set -e
...

Also avoid explicitly using Bash if you don't use any Bash-only features, instead use /bin/sh as the shell. The latter might be Bash or another POSIX-compliant shell, and is guaranteed to exist on UNIX systems, while /bin/bash might not.

Unnecessary use of class

class FileHandler contains no member variables and only static member functions. There is no advantage to put those in a class, just put them outside, and keep them static. You could consider putting them in a namespace instead, but if these functions are only going to be used by other code in the same .cpp file, I don't see a reason for that either.

Avoid dealing with differences in path separators

While Windows normally uses backslashes as a separator of path components, it also accepts forward slashes. But even better, since you are using std::filesystem, store paths in std::filesystem::path, which handles concatenation of path components for you. For example:

namespace fs = std::filesystem;

static fs::path GenerateFilename(const fs::path& directory, const fs::path& filename) {
    return directory / filename;
}

Of course now this function is so simple that you don't even need it anymore. A lot of your code simplifies by using std::filesystem::path to hold everything that is a directory or filename. Only convert them to std::string if absolutely necessary.

Avoid storing filenames in a temporary vector

There is no point in having GetFilenames() loop over the directory, storing all filenames in a std::vector, and then have the code in main() loop again over that vector. Instead, just loop over the directory in main():

const std::filesystem::path directory = argv[1];
...
for (auto const& dir_entry : directory) {
    std::ifstream inputFile(dir_entry.path());
    ...
}

Is class config necessary?

Similarly to storing filenames in a temporary vector, do you actually need to store the configuration into an instance of class config before you write it out? It seems to me that you could write directly to the output file while you are reading the input:

for (auto const& dir_entry : directory) {
    outputFile << '[' << dir_entry.path().string() << "]\n";
    
    std::string line;
    std::ifstream inputFileopen(dir_entry.path());
    while (std::getline(inputFile, line)) {
        auto keyValue = StringTransformer::SplitOn(line, ':');
        outputFile << keyValue.first << '=' << 'keyValue.second' << '\n';
    }
}

Missing error checking

At no point do you check whether input and output files are read or written to correctly, and neither do you validate the keys and values.

Make sure you check after reading the last line of an input file that inputFile.eof() == true, and after writing the output close() it and then check that outputFile.good() == true. If any of those are false, print an error message to std::cerr and exit the program with EXIT_FAILURE.

Also consider what happens if the key in one of the input files contains an =, what to do with leading/trailing spaces, if there are differences in how booleans, numbers and strings are represented in prop vs. INI files. Can prop files have comments in them? What about lines without a : or with an empty key or value?

\$\endgroup\$
3
\$\begingroup\$

The code mostly looks good.

You almost certainly want to make the one-argument constructors explicit, to avoid the compiler considering any surprising implicit conversions. These constructors appear unused, so perhaps removing them would be better.

The serialize() member function shouldn't need to modify the object, so make it const.

The parser seems a little optimistic - we should probably be reporting errors where we don't find any = in a non-header line (and don't repeat the find()). Also, check whether there's any comment syntax we need to strip out.

We have included <filesystem>, so we should be using that for constructing paths, rather than assuming each supported platform has exactly one directory separator character (IIRC, Windows platforms can use / and \ interchangeable as separators). Don't reinvent wheels when we don't have to!

Why is StringTransformer a class? It has no member objects, and I think a namespace would be more appropriate here.

The write interface (which produces a std::string containing the entire contents of the file) is unwieldy and wastes memory. If the (unused) operator<< was modified to write in the desired format, creating a large string would be unnecessary and the calling code would be simpler.

There's quite a few unused members in config. Certainly, I was able to eliminate iterator, begin(), end(), remove() and all the constructors without affecting compilation. If these haven't been been tested, then they should be simply deleted.

Error message should go to std::cerr, not to the output stream.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.