4
\$\begingroup\$

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?

\$\endgroup\$
1
  • \$\begingroup\$ I don't understand test3. getProperty don't throw PropertyDoesAlreadyExistException \$\endgroup\$ – shanif Apr 17 '20 at 18:54
3
\$\begingroup\$

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).

\$\endgroup\$
2
  • \$\begingroup\$ That micro-optimization may even be a pessimization because O(log n) is not faster than O(n), only in a very simplified interpretation. My gut feeling is that the number of different properties is low, so these asymptotic behaviours don't apply. \$\endgroup\$ – uli Apr 17 '20 at 19:46
  • \$\begingroup\$ I extended the problem statement: The number of properties is usually very low (less than 10). It is not expected to have a high count here. \$\endgroup\$ – Vertexwahn Apr 17 '20 at 20:57
3
\$\begingroup\$

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)
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ Sorry for not providing more context: The propose of the class is to map properties defined in an Xml document to properties that can be used within a C++ programm e.g. <Property name="a" type="float" value="10" /> \$\endgroup\$ – Vertexwahn Apr 17 '20 at 22:23
  • 1
    \$\begingroup\$ @Vertexwahn boost::ptree (aka property tree) does it for you. It has parsing tools (read and write) that support xml format. \$\endgroup\$ – ALX23z Apr 18 '20 at 6:16
  • \$\begingroup\$ @Vertexwahn if you still want to use your own implementation, consider switching from the variant to just a std::string and an enum that identifies what type it is for streaming. Also, disabling overwriting options is too limiting. At least make two functions one that doesn't overwrite for most uses and one does for more general purpose. \$\endgroup\$ – ALX23z Apr 18 '20 at 6:27
  • \$\begingroup\$ Had a look on boost::ptree - i do not know the format of the XML document - I just now there are properties - hence I do not have a path I can specify. I know my problem statement did not include this restriction - sorry. \$\endgroup\$ – Vertexwahn Apr 18 '20 at 21:51
  • \$\begingroup\$ @Vertexwahn then how do you intend to use your class if you don't know the names/identifiers of the properties? At any rate there are iterators one can use to iterate over all properties. \$\endgroup\$ – ALX23z Apr 18 '20 at 22:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.