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

Question

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 <algorithm>
#include <iostream>
#include <string>

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;
    count++;
    if( count % 3 == 0){
        key = "max_search_depth";
        value = "10";
    }
    else if( count % 3 == 1){
        key = "enable_recursion";
        value = "false";
    }
    else {
        key = "job_name";
        value = "Khaleesi";
    }


    // key and value have now been set
    // based on what was parsed in from the file


    // check if value is numeric
    char *numeric_check;
    int numeric_value = static_cast<int>(strtod( value.c_str(), &numeric_check ));


    // value is a boolean
    if( value == "true" || value == "false") {

        //initalize object and return it
        declaration_bool *d_bool = new declaration_bool;
        d_bool->key = key;
        d_bool->value = (value == "true");
        return dynamic_cast<declaration*>(d_bool);
    }

    // value is a number
    else if( *numeric_check == '\0'  ) {
        declaration_int *d_int = new declaration_int;
        d_int->key = key;
        d_int->value = numeric_value;
        return dynamic_cast<declaration*>(d_int);
    }

    // value is a string
    else {
        declaration_string *d_string = new declaration_string;
        d_string->key = key;
        d_string->value = value;
        return dynamic_cast<declaration*>(d_string);
    }

    // side note: I am aware of this memory leak. For sake of simplicity
    // I have not worried about deleting the declaration structs. In acctual
    // code this would be properly handled.
}

// irrelevent and used only for illustrative purposes
void setRecursion( bool enabled ){}
void setDepth( int levels ){}
void statusReport( std::string job_name ){}


int main (){

    /*shhhh*/int lines_remaining = 3;/* Again, only to keep it simple. Not real code*/

    // while file still has lines to read in
    while( lines_remaining-- ){

        declaration *line = readLine();

        if(line->key == "enable_recursion" ){
            // I know I can expect a bool value next due to the "enable_recursion" key,
            // so I can check to ensure it is indeed a bool and be safe, but the general
            // idea now is to just:
            declaration_bool *db = dynamic_cast<declaration_bool*>(line);

            // and just do whatever I wanted to do with that value;
            setRecursion( db->value );
        }

        else if(line->key == "max_search_depth" ){
            declaration_int *di = dynamic_cast<declaration_int*>(line);
            setDepth( di->value );
        }

        else if(line->key == "job_name" ){
            declaration_string *ds = dynamic_cast<declaration_string*>(line);
            statusReport( ds->value );
        }

        else {
            // unknown/bad key found. deal with it.
        }

    }
}

Answer 1

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 <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";
    }
}

A quick rundown of how this works: we have a boost::variant<bool, int, std::string>. 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<declaration> 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 <memory>

std:unique_ptr<declaration> 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 is really required is more akin to union, which has its own problems. Thus the choice of using boost::variant in the solution above: it can be used in similar situations, but is generally easier and less error prone to work with.

As a final word, this solution itself still isn't perfect. It is still relatively easy for a someone using the above code to forget to do the proper checks to make sure they're casting things to the correct type. Due to the nature of the way the data is stored, this isn't really something that can be worked around in the code that reads it in. The better option would be to modify the data itself: this could be as simple as adding an extra field with the value type or prefacing the value with its type, for example:

max_search_depth integer:10
enable_recursion boolean:false
job_name string:Khalessi

Then, while parsing the file, the type information would be there along with the value. If you are able to modify the file format, this may be the simplest solution.

Answer 2

Your main issues is that on input you have different types for each key. So you spend time building a generic class hierarchy to hold each of the types. But the main code we see that each specific key expects a specific type of value and thus you try and decode the generic type based on the key value. Personally I would reverse this pattern.

As you read the input. When each key is found extract the type you want and call the appropriate handler method.

Your second problem is the number of keys that you will eventually have. Yes you should re-write the code to be data driven. That means you to store the mappings of key to action in data not the code. Thus making the driver code short east to read and the same in every case. The only interesting part of the code becomes the data.

Below I have mapped keys to actions in the data structure actionMap.

#include <map>
#include <algorithm>
#include <string>
#include <memory>
#include <functional>
#include <iostream>

void setRecursion( bool enabled ){} 
void setDepth( int levels ){} 
void statusReport( std::string job_name ){}

template<typename R> R readA(std::istream& stream)
{
    R   value;
    stream >> value;
    return value;
}

int main()
{
    std::map<std::string, std::function<void(std::istream&)> > const  action
    {
        {"enable_recursion", [](std::istream& value){setRecursion(readA<bool>(value));}},
        {"max_search_depth", [](std::istream& value){setDepth(readA<int>(value));}},
        {"job_name",         [](std::istream& value){statusReport(readA<std::string>(value));}}
    };

    std::string     key;
    while(std::cin >> key)
    {
        auto find = action.find(key);
        if (find != action.end())
        {
            // If we find an action in the action map
            // then call it.
            find->second(std::cin);
        }
        else
        { 
            // Unknown action deal with it.
        }
        // Ignore any unused values on the line
        // Thus each iteration of the loop starts at
        // the beginning of a new line.
        std::string     ignoreLine;
        std::getline(std::cin, ignoreLine);
    }
}

The above is C++11. But it is easy to re-write for C++03. The main features of C++ here are syntactic sugar over well known easy to use C++03 ways of doing things (that take slightly more typing).

Comments on your code:

while( lines_remaining-- ){

    declaration *line = readLine();

This is usually an anti-pattern. What happens if the readLine() fails? You should check for that.

The normal pattern would be:

declaration* line
while(lines_remaining-- && ((line = readLine()) != NULL)){
   // The loop is only enetered
   // if the loop succeded.

You will see this pattern in most languages. The read is always part of the test to see if it we do something next (do work/enter a loop).

You mention you know it is a resource problem

declaration *line = readLine();

but even for a simple example this leak can easily be solve.

std::unique_ptr<declaration>  readLine();

// STUFF
std::unique_ptr<declaration>  line = readLine();

The use of smart pointers to indicate ownership symantics so should be automatic. The use of smart pointers to make sure that you don't leak resources should be automatic.

If you have to force polymorphsm on the code use the virtual destructor. You need one anyway. As a calling the destructor via the base class pointer is UB if the destructor is not virtual.

Potentially you can make it pure as a sub-class will automatically define its own destructor. But this will make sure you can not instantiate any version of the abstract base class.

struct declaration {

    std::string key;

    virtual ~declaration()  = 0;
};

But the above is a bad idea (you are not really using the polymorphic properties anyway. Also when you see dynamic_cast<> in the code there is usually something wrong with the design.

declaration_bool *db = dynamic_cast<declaration_bool*>(line);

Actions based on specific types should be accessed via a call to a virtual function. Where each type then handles that action in their own special way.

Also you should check that the result of the cast is not NULL.

You don't need to use dynamic_cast<> when returning.

    return dynamic_cast<declaration*>(d_int);

the result is already a declaration*.