Skip to main content
Code Review

Return to Answer

added an actual code review
Source Link
Yuushi
Yuushi
  • 11k
  • 2
  • 30
  • 66

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.

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.

Source Link
Yuushi
Yuushi
  • 11k
  • 2
  • 30
  • 66

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.