Requesting Feedback on a C++ Config Class

Started by
7 comments, last by rpiller 10 years, 4 months ago

Hey guys, I'm attempting to learn C++ coming from more of a PHP/Java/C# world and was hoping to get some feedback from some more experienced programmers on this Config class I've come up with. It seems to work exactly as I intend, but I can't shake this feeling that I'm not following some C++ best practices, or that it's somehow unsafe.

Basically what I'm doing is using boost::any to wrap a boost::shared_ptr to create a heterogeneous map of config objects. These are designed to be mostly simple types such as int, float, bool, but I would also like to support dynamic arrays (vector) and maybe simple structures such as Vector3D or Matrix values.

Cvar is supposed to operate as a read only pointer to a value so that I can modify config values at runtime, yet not change config values through the Cvar interface.

Any feedback on this, or things I might have missed would be greatly appreciated. I don't know if I'm just overthinking it, but the class seems like it's too small to be done safely and correctly.

A little syntax of how it's supposed to work.


ConfigMap config;
config.Set<int>("runningSpeed", 10);

Cvar<int> speed = config.Get<int>("runningSpeed", 1);
cout << speed; //Outputs 10

config.Set<int>("runningSpeed", 20);
cout << speed; //Outputs 20

int playerXPos = 5;
playerXPos += speed;

cout << playerXPos; //Outputs 25


And finally the code


template <class TYPE>
class Cvar {
public:
    Cvar() : value(NULL) {}
    Cvar(boost::shared_ptr<TYPE> t) : value(t) {}
            
    inline friend std::ostream& operator<<(std::ostream& os, const Cvar& dt) {
        os << *dt.value;
        return os;
    }
    inline friend std::istream& operator>>(std::istream& is, const Cvar& dt) {
        *dt.value >> is;
        return is;
    }
    inline operator TYPE() { return *value; }
private:
    boost::shared_ptr<TYPE> value;
};
        
class ConfigMap {
    typedef std::unordered_map<std::string, boost::any> ConfigStore;
public:
    template <class TYPE>
    void Set(std::string key, TYPE val) {
        if(configStore.find(key) == configStore.end()) {
            configStore.insert(std::make_pair(key, boost::shared_ptr<TYPE>(new TYPE(val))));
        } else {
            boost::shared_ptr<TYPE> value = boost::any_cast<boost::shared_ptr<TYPE>>(configStore[key]);
            *value = val;
        }
    }
            
    template <class TYPE>
    const Cvar<TYPE> Get(std::string key, TYPE defaultValue) {
        if(configStore.find(key) == configStore.end()) {
            Set(key, defaultValue);
        }
                
        boost::shared_ptr<TYPE> value = boost::any_cast<boost::shared_ptr<TYPE>>(configStore[key]);
        return Cvar<TYPE>(value);
    }
private:
    ConfigStore configStore;
};

Advertisement
What's the point of class Cvar? it holds a value without any useful behaviour. In fact, it has dangerous behaviour: client code can change or corrupt the configuration without proper thread synchronization and arbitrarily. Copying everything in Get() and Set() with proper locking would be better.

Omae Wa Mou Shindeiru

I think the main point was so he can overload << & >>. I don't know why one would need to do that though, since I could just do the following to get the same result.

int speed = config.Get<int>("speed", 1);
 
cout << speed;

You don't need Cvar since you are using boost::any. If you didn't want to use boost at all then you would need a base class like Var that isn't templated and then a derived class like Cvar that is templated and stores your data and in your Config class store a map of Var pointers but create Cvar objects. This way you don't have to use boost::any which from what I've seen is a lot of code and probably overkill for what you need in this specific example.

Whats the point of reinventing ini configuration files, there is a Windows API that allows you to read these settings from a file.

Worked on titles: CMR:DiRT2, DiRT 3, DiRT: Showdown, GRID 2, theHunter, theHunter: Primal, Mad Max, Watch Dogs: Legion

I think the main point was so he can overload << & >>. I don't know why one would need to do that though, since I could just do the following to get the same result.


int speed = config.Get<int>("speed", 1);
 
cout << speed;

You don't need Cvar since you are using boost::any. If you didn't want to use boost at all then you would need a base class like Var that isn't templated and then a derived class like Cvar that is templated and stores your data and in your Config class store a map of Var pointers but create Cvar objects. This way you don't have to use boost::any which from what I've seen is a lot of code and probably overkill for what you need in this specific example.

If you look at his example usage, you'll see that the Cvar object is a reference to the actual value stored in the ConfigMap object. When he change the value of some parameter in the ConfigMap object, all the Cvar handles to that parameter also change.

Not saying the solution is fine as it is, but returning a int is not equivalent to what his class is doing.

In the examples he gave it would seem to be the same. He's not doing anything special with the return value. I guess he should show us an example as to why he needs the Cvar class.


Whats the point of reinventing ini configuration files, there is a Windows API that allows you to read these settings from a file.

He may not be using this to read from file but adding some console functionality to his game? He may not be using Windows.

In the examples he gave it would seem to be the same. He's not doing anything special with the return value. I guess he should show us an example as to why he needs the Cvar class.

His exampe requests the value of runningSpeed from the ConfigMap object into the speed object and prints it. After updating the runningSpeed property of the ConfigMap object, the speed object is printed again with the new value.

I think its not a good design to stuff these any things into a single container. Just use one map for each type you want to store, though just having int and string may be enough as you can just use 0,1 for bool, use fixed point in the rare case you need a float and use 2 or 3 values with appended x, y or z if you need a vector.


His exampe requests the value of runningSpeed from the ConfigMap object into the speed object and prints it. After updating the runningSpeed property of the ConfigMap object, the speed object is printed again with the new value.

My example way has the same output. I didn't give my entire example to match his as I thought it was implied the route I was saying he could go based on the code snippet and the text description of the changes that would have to happen. I understand he returns an object, but I'm saying it's not really needed as the way I did it gives the same output to the screen does it not?

Again, my explanation of my example means it would change the way he's currently coding his config class. I'm not sure if you think I think my example would work with his current config class setup because I know it wont. My explanation was how he would have to change his setup, to make it work like my example I gave, which would provide the same output to the screen, which would remove the need for boost:any and having any kind of object returned and having to overload any operators. That's all I was trying to show.

This topic is closed to new replies.

Advertisement