HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Trying to find a good design for reading in values of different types from a file

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
cppcodereviewstackoverflowc++
typesfindreadingfiledesigntryingdifferentforgoodvalues

Problem

I have implemented the following code which will be used to read in what are essentially key-value pairs from a file.

an example input file may look like

max_search_depth 10
enable_recursion false
job_name Khalessi


my main issue with this code is that I wish to return variables of different types based on the text in the file. max search depth is best suited as an int, while job name is a string, and enable recursion is a bool

currently I solve this issue by sub classing the declaration struct, but i wonder if there are other options for achieving this which put less onus on the calling function to know what variable type it will be receiving with any given key.

I am hoping to purpose this code for a few different areas in my application, so I would like to keep it as generic as possible.

my secondary concern is with the section involving if(key == a_particular_key) { //do whatever }
There are potentially 100's of possibilities of what a key could be, so I am sure there must be a more elegant way of finding the key and directing program flow accordingly, but as of yet I have not learned a more elegant solution

tertiary concern of minor importance: do i really need void v() in the parent class to use polymorphism?

Thanks a ton for looking at my code!

```
#include
#include
#include

struct declaration {

std::string key;

// need at least one virtual method to enable polymorphism?
// compiler errors will result if v() is not present.
virtual void v(){}
};

// Specializations of the declaration struct for their respective types

struct declaration_int : public declaration { int value; };

struct declaration_string : public declaration { std::string value; };

struct declaration_bool : public declaration { bool value; };

declaration* readLine(){

std::string key, value;

// Irrelevent code. Simulates reading in a line from a file. It's only
// here to keep the code consise and relevent.
static unsigned int count = 0;

Solution

There's quite a bit to comment on here, however, it's late, so I'm going to post a solution to your problem, and then try and quickly explain it. Hopefully I can edit this post with more details soon.

Crux of the solution: there are two major problems here. Firstly, the keys are not stored in any kind of way that makes their associated values easy to access. The second, and more difficult problem, is that the caller must know in advance what type a given key maps to.

We'll solve the first problem with a std::map. The second problem requires more sophisticated machinery. In this case, we'll use a boost::variant, which is a discriminated union type: it can store any one of a number of types (which are specified via template):

#include 
#include 
#include 
#include 
#include 
#include 

#include "boost/variant.hpp"

enum contains_type
{
    BOOL_T, 
    INT_T, 
    STRING_T, 
    NONE_TYPE 
};

struct type 
{
private:

    typedef boost::variant variant_t;
    typedef std::pair value;
    std::map map_;

public:

    typedef std::map::const_iterator const_iterator;

    void add_value(const std::string& key, const std::string& value)
    {
        variant_t var;
        contains_type c;

        if(value == "true") {
            var = true;
            c = BOOL_T;
        }
        else if(value == "false") {
            var = false;
            c = BOOL_T;
        }
        else if(std::all_of(value.begin(), value.end(),
                            [](char c) { return std::isdigit(c); })) {
            var = std::atoi(value.c_str());
            c = INT_T;
        } else {
            var = value;
            c = STRING_T;
        }

        map_.insert(std::make_pair(key, std::make_pair(std::move(var), c)));
    }

    const_iterator get_value(const std::string& key) const
    {
        return map_.find(key);
    }

    bool get_value_bool(const std::string& key) const {
        auto it = map_.find(key);
        return boost::get(it->second.first);
    }

    int get_value_int(const std::string& key) const {
        auto it = map_.find(key);
        return boost::get(it->second.first);
    }

    std::string get_value_string(const std::string& key) const
    {
        auto it = map_.find(key);
        return boost::get(it->second.first);
    }

    contains_type get_type(const std::string& key) const
    {
        auto it = map_.find(key);
        if(it == map_.end()) return NONE_TYPE;
        return it->second.second;
    }

    const_iterator begin() const
    {
        return map_.begin();
    }

    const_iterator end() const
    {
        return map_.end();
    }
};

int main()
{
    type t;
    t.add_value("enable_recursion", "true");
    t.add_value("max_search_depth", "10");
    t.add_value("job_name", "foobar");
    contains_type c = t.get_type("enable_recursion");
    if(c == BOOL_T) {
        bool b = t.get_value_bool("enable_recursion");
        std::cout << "enable_recursion: " << b << "\n";
    }
}


A quick rundown of how this works: we have a boost::variant. This
can have any one of those types assigned to it. We also have an enum which will be stored
with it that will keep track of what type it is assigned. We store both of these, along with the std::string key in a std::map. That's basically all of the top stuff taken care of.

add_value simply inserts the key into the map, converts the value string into the correct type, and stores the value/type pair under that key in the map.

The other functions are mainly for convenience.

This may be too advanced for you currently, but it does solve both problems. As I said, I'll try and update this post with more information (and some actual feedback on your code) when I get a chance.

Edit: Ok, now for an actual code review.

Your design utilises dynamic_cast in a number of places. You can actually replace all of these in the readLine function with static_cast since we know that it will not fail here. This will also not incur the performance penalty of having to do the type check.

The memory leak you talk about shouldn't really be something that is an afterthought. In this situation, if you have access to C++11, you should be using RAII to stop memory leaks and to solidify ownership:

#include 

std:unique_ptr readLine()
{
    //...
}


The main difficulty with this design is, as I've said previously, that the user of this has to know in advance what type you're getting back is. Unfortunately, this is the opposite of the problem that polymorphism is trying to solve. With polymorphism, the idea is that you get back an instance of some class that conforms to an interface, but that you should not care what the underlying type is. In this case, you're returning a base class, but you are required to know what its underlying subclass is. Because of the nature of the problem, and the fact that C++ cannot overload functions by return type, this isn't really the correct way to go: classic OOP won't work here. What

Code Snippets

#include <algorithm>
#include <cctype>
#include <iostream>
#include <map>
#include <string>
#include <utility>

#include "boost/variant.hpp"

enum contains_type
{
    BOOL_T, 
    INT_T, 
    STRING_T, 
    NONE_TYPE 
};

struct type 
{
private:

    typedef boost::variant<bool, int, std::string> variant_t;
    typedef std::pair<variant_t, contains_type> value;
    std::map<std::string, value> map_;

public:

    typedef std::map<std::string, value>::const_iterator const_iterator;

    void add_value(const std::string& key, const std::string& value)
    {
        variant_t var;
        contains_type c;

        if(value == "true") {
            var = true;
            c = BOOL_T;
        }
        else if(value == "false") {
            var = false;
            c = BOOL_T;
        }
        else if(std::all_of(value.begin(), value.end(),
                            [](char c) { return std::isdigit(c); })) {
            var = std::atoi(value.c_str());
            c = INT_T;
        } else {
            var = value;
            c = STRING_T;
        }

        map_.insert(std::make_pair(key, std::make_pair(std::move(var), c)));
    }

    const_iterator get_value(const std::string& key) const
    {
        return map_.find(key);
    }

    bool get_value_bool(const std::string& key) const {
        auto it = map_.find(key);
        return boost::get<bool>(it->second.first);
    }

    int get_value_int(const std::string& key) const {
        auto it = map_.find(key);
        return boost::get<int>(it->second.first);
    }

    std::string get_value_string(const std::string& key) const
    {
        auto it = map_.find(key);
        return boost::get<std::string>(it->second.first);
    }

    contains_type get_type(const std::string& key) const
    {
        auto it = map_.find(key);
        if(it == map_.end()) return NONE_TYPE;
        return it->second.second;
    }

    const_iterator begin() const
    {
        return map_.begin();
    }

    const_iterator end() const
    {
        return map_.end();
    }
};

int main()
{
    type t;
    t.add_value("enable_recursion", "true");
    t.add_value("max_search_depth", "10");
    t.add_value("job_name", "foobar");
    contains_type c = t.get_type("enable_recursion");
    if(c == BOOL_T) {
        bool b = t.get_value_bool("enable_recursion");
        std::cout << "enable_recursion: " << b << "\n";
    }
}
#include <memory>

std:unique_ptr<declaration> readLine()
{
    //...
}
max_search_depth integer:10
enable_recursion boolean:false
job_name string:Khalessi

Context

StackExchange Code Review Q#29474, answer score: 2

Revisions (0)

No revisions yet.