PropertySet implementation

Question

Problem Statement:

Store properties with a certain name (key), type and value. The key is always a character literal and the value can be of type bool, int, float, double, char, std::string, Vector2f, Vector2d, Color3f. It should be possible to add new properties and to retrieve the values of already existing properties. It is not expected that the value of a property is changed after adding it to the property set. The names of properties are unique and cannot be used multiple times (i.e. there can not be two properties with the same name). The number of properties is usually very low (less than 10 - it is not expected to have a high count here).

Implementation:

Here is a snippet of my proposed C++17 solution:

#include <map>
#include <sstream>
#include <string>
#include <variant>

...

class PropertySet {
public:
    template<typename ValueType>
    void addProperty(const std::string &name, const ValueType value) {
        if (hasProperty(name)) {
            // C++20 or https://github.com/fmtlib/fmt
            // std::string msg = std::format("Property with name '{}' does 
            // already exist and its value is '{}'", name, value);
            std::stringstream ss;
            ss << "Property with name '"
               << name.c_str()
               << "' does already exist and its value is '"
               << value << "'";
            throw PropertyDoesAlreadyExistException(ss.str());
        } else {
            values_[name] = value;
        }
    }

    template<typename ValueType>
    ValueType getProperty(const std::string &name) const {
        if (hasProperty(name)) {
            return std::get<ValueType>(values_.at(name));
        } else {
            std::stringstream ss;
            ss << "Property with name '" << name.c_str() << "' does not exist";
            throw PropertyDoesNotExistException(ss.str());
        }
    }

    // A defaultValue can be provided in the case a property does not exist
    template<typename ValueType>
    ValueType getProperty(const std::string &name,
                          const ValueType defaultValue) const {
        if (hasProperty(name)) {
            return std::get<ValueType>(values_.at(name));
        } else {
            return defaultValue;
        }
    }

    bool hasProperty(const std::string &name) const {
        return values_.find(name) != values_.end();
    }

private:
    typedef std::variant<bool,
            int,
            float,
            double,
            char,
            std::string,
            Vector2f,
            Vector2d,
            Color3f> VariantType;

    typedef std::map<std::string, VariantType> MapType;

    MapType values_;
};

Test 1:

TEST(PropertySet, TestBasicTypes) {
    PropertySet ps;

    ps.addProperty("a", 1);
    ps.addProperty("b", 1.0f);
    ps.addProperty("c", 1.0);
    ps.addProperty("d", 'a');
    ps.addProperty("e", std::string("Hello World!"));
    ps.addProperty("f", true);

    EXPECT_EQ(ps.getProperty<int>("a"), 1);
    EXPECT_EQ(ps.getProperty<int>("a"), 1);
    EXPECT_EQ(ps.getProperty<float>("b"), 1.0f);
    EXPECT_EQ(ps.getProperty<double>("c"), 1.0);
    EXPECT_EQ(ps.getProperty<char>("d"), 'a');
    EXPECT_EQ(ps.getProperty<std::string>("e"), "Hello World!");
    EXPECT_EQ(ps.getProperty<bool>("f"), true);
}

Test 2:

TEST(PropertySet, WhenPropertyDoesNotExist_Then_ReturnDefaultValue) {
    PropertySet ps;
    EXPECT_THAT(ps.getProperty("notExistingProperty", 42), ::testing::Eq(42));
}

Test 3:

TEST(PropertySet, WhenSamePropertyIsAddedTwice_ThenThrowExceptionAndExpectProperErrorMessage) {
    PropertySet ps;
    ps.addProperty("a", 1);

    EXPECT_THROW(ps.addProperty("a", 2), PropertyDoesAlreadyExistException);

    try {
        ps.getProperty<int>("a");
    }
    catch (PropertyDoesAlreadyExistException &ex) {
        EXPECT_THAT(std::string(ex.what()), 
                    ::testing::Eq("Property with name 'a' does not exist and its value is '1'"));
    }
}

I have more tests in place.

Questions:

Implementation

• Should getProperty return a (const) reference?
• Should I switch for name to const char* instead of using a std::string?
• From a C++17 perspective: Are there more modern features of the language that I should use?
• Should const T defaultValue be const T& defaultValue?
• Is it clear from the name what is the idea of the defaultValue? Should I add a comment here?

Testing

• Should Test 3 be split into two tests?
• Should I use EXPECT_THAT everywhere (EXPECT_THAT vs EXPECT_EQ)?
• Are there any edge cases for which the implementation fails?

Do you have other feedback, improvements for the implementation and testing?

Answer 1

Exceptions

It is better to pass the property name to the exception constructor and let the exception class define the message. It is better because:

• It's more DRY. If you have to throw that exception from another function you need to create the same message.
• Single responsibility
• Easier to test - only need to test the exception and the property name. The test will not break if you decide to change the message.
• Logging - when your exceptions connected to some logging mechanism and a tool. You can search for this specific exception and specific property name.

Naming

I think a good name for a map is a name describing the key and the value. I think values_ should be called propertyName2Value.

Suggested Additions

• Add a specific exception to the case get is called with the wrong type
• implement operator[]

Performance

It is probably a micro-optimization but map operations runtime complexity are O(log N) where unordered_map operations are O(1).

Answer 2

Here is a minor thing that you should take into consideration.

In C++17 we have the lovely string_view type that is a const std::string, so your code on

void addProperty(const std::string &name, const ValueType value)

Should be

void addProperty(std::string_view &name, const ValueType value)

Answer 3

Prefer to use the modern version of typedef (which is using).

    typedef std::variant<bool,
            int,
            float,
            double,
            char,
            std::string,
            Vector2f,
            Vector2d,
            Color3f
            > VariantType;

Now looks like:

    using VariantType = std::variant<bool,
            int,
            float,
            double,
            char,
            std::string,
            Vector2f,
            Vector2d,
            Color3f
            >;

---

Should getProperty return a (const) reference?

Yes.

Should I switch for name to const char* instead of using a std::string?

No. At some point you have to build a string (to have a key to compare against the map). May as well be as the parameter.

From a C++17 perspective: Are there more modern features of the language that I should use?

No this is fine.

Should const T defaultValue be const T& defaultValue?

Yes.

Is it clear from the name what is the idea of the defaultValue? Should I add a comment here?

I don't think it needs a comment.

Answer 4

I use something similar to what you have but with certain deviations. In fact, I have two classes with slightly different interfaces and slightly different purposes. So I'd like to advise on general design direction as I believe here lies the main issues.

1) You should try and be clear as what the class does and what it's purpose: what rules it ought to abide by and why.

For example, in your case in one place one could store the a property as float and another try to read it as double which would result in error. Do you even want it to be an error? Why not store all floating points as a long double in the first place? Same problem with bool, char, int and despite all these three overlapping options you don't have one for int64 or even std::size_t for various architectures.

One of the main principles of programming is KISS and it is generally preferable to stick to it instead of making over-complicated "smart" solutions that result in bugs and errors. One of the two classes that I use has the following purpose:

a. provide configuration parameters to all classes that have access to it. source of these configuration parameters can be either user / external class / configuration file / command line / and even configurable by a parent class.

To accommodate such a large variety of uses one ought to make it as compatible as possible. Thus it is essentially a map<string,string> (boost::ptree is a better version of it) and if one wants to store double, int64 or whatever just serialize it into a string and backwards on reading. One only needs to write conversion functions back and forward. Nothing too complex.

For instance, in your case, it is hard to obtain data from configuration file / user that isn't a string. How does the configuration file / user lets the program know what type the data is? is it float? bool? char? vector2d? How does one distinguish the types from the text? It should be as simple as possible.

b. it needs to be thread-safe as multitude classes can use it simultaneously (even if mostly use it for reading, some may still perform a write). Thus a mutex is used to guard its methods.

In your case, if one added a property in one thread while in another thread one reads a completely different property it will cause a data race and it results in rare but troublesome errors.

2) The other class that I use deviates from your class' purpose more significantly but has some nice extra features that you should consider to incorporate.

It isn't property set but rather a "resource map" and/or "shared variables". Imagine you have following scenario: variety of your classes use a class, say GPU_Context, and you want to instantiate it only once and share it among all other classes. To do that I simply implemented a method that performs unique instantiation (according to the user provided method) in the "resource map" class and shared the instance of "resource map" for all relevant classes.

Unlike "property set" the "resource map" stores shared pointers to a virtually destructible class (I suppose one can utilize std::any instead for this purpose in C++17) so they can be whatever. And its main functions are init_resource (initiates the resource; does nothing if the resource is already set or already being initialized), ser_resource (if it is already being initialized or set then it throws exception), get_resource (with option for default initialization; otherwise throws exception if the resource is not already set), wait_for_resource (waits until the resource is set). It is not hard to implement these with usage of std::promise and std::shared_future.