4
\$\begingroup\$

I wrote a function that accepts a destination vector and a vector of all the types that are currently available. Its job is to iterate over all the available types (strings) and for each type, if it's required, the respective derived class should be added to the vector. I also want to enforce a constraint that only one of each derived can be added to the vector (in the future, this constraint might change and some types will be able to appear in the destination more than once).

I feel like there might be a more elegant way to solve it, and would appreciate a review on how I can better it. (I considered using an unordered set for the version with constraint of 1 per type, but then how would I ensure that I found exactly one of a type in the available vector? Once I erase a type from the set I can't track if I encountered another one, which is an error state.)

This is a simplistic version of the code (C++14):

#include <string>
#include <vector>
#include <unordered_map>
#include <cassert>
#include <memory>

class Base {
public:
    virtual ~Base() {}
};

class DerivedA : public Base {
public:
    static std::string getType() {
        return "DerivedA";
    }
};

class DerivedB : public Base {
public:
    static std::string getType() {
        return "DerivedB";
    }
};

class DerivedC : public Base {
public:
    static std::string getType() {
        return "DerivedC";
    }
};

void fillRequired(std::vector<std::unique_ptr<Base>>& dstVector, const std::vector<std::string>& availableTypes)
{
    std::unordered_map<std::string, int> requiredTypeCount = {
        {DerivedA::getType(), 0}, {DerivedB::getType(), 0}, {DerivedC::getType(), 0}};

    for (const auto& availableType : availableTypes) {
        auto it = requiredTypeCount.find(availableType);
        if (it != requiredTypeCount.end()) {
            requiredTypeCount[availableType]++;
            if (availableType == DerivedA::getType()) {
                dstVector.emplace_back(std::make_unique<DerivedA>());
            } else if (availableType == DerivedB::getType()) {
                dstVector.emplace_back(std::make_unique<DerivedB>());
            } else if (availableType == DerivedC::getType()) {
                dstVector.emplace_back(std::make_unique<DerivedC>());
            }
        }
    }

    for (const auto& requiredType : requiredTypeCount) {
        assert(requiredType.second == 1 && "Each type must be present exactly once");
    }
}

Thanks (:

\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

There are various ways to approach this problem. What the best way is will depend on things like how often this function is called, and how many elements requiredTypeCount and availableTypeCount will have in a real program. If you wish for elegance, then I would want to write it like so:

void fillRequired(std::vector<std::unique_ptr<Base>>& dstVector,
                  const std::set<std::string>& availableTypes)
{
    static const std::set<std::string> requiredTypes = {
         DerivedA::getType(), DerivedB::getType(), DerivedC::getType()
    };

    std::set_intersection(requiredTypes.begin(), requiredTypes.end(),
                          availableTypes.begin(), availableTypes.end(),
                          std::back_inserter(dstVector));
}

I changed the type of the parameter availableTypes to a std::set; this doesn't allow duplicates, so this function doesn't have to worry about that anymore. You could always add another function that builds this std::set from a std::vector and that does assert() when it finds duplicates.

Then requiredTypes is also changed to a std::set. But it doesn't need to count anymore, so we can make it static const; this means it doesn't have to build this set from scratch every time the function is called.

Now that we have two ordered containers without duplicates, we can use std::set_intersection() to find the intersection between the two sets. We then use std::back_inserter() to push the elements that are in both sets into dstVector.

But oops, that just tries to insert std::strings into dstVector, but instead we want to insert pointers to newly created objects. Unfortunately there is no way in the standard library that we can use to pass a lambda instead, but Boost has a handy function_output_iterator that we could use:

std::set_intersection(…,
                      boost::function_output_iterator(
                          [&](const std::string& name) {
                              if (name == DerivedA::getType()) {
                                  dstVector.emplace_back(std::make_unique<DerivedA>());
                              } else …
                          }
                      )
);

You could also write your own output iterator if you don't want to rely on Boost (it's not very hard). But this isn't so elegant anymore. I would at the very least create a separate function that, given a std::string, returns a std::unique_ptr<Base>. That would simplify both your original code and the code I suggested here.

Some other ideas:

  • Consider returning a std::vector<std::unique_ptr<Base>> instead of passing it as an out parameter.
  • Consider using typeid() instead of having a getType() function. Comparing the result of typeid() might also be faster than comparing std::strings.
  • assert() becomes a no-op when NDEBUG is defined. This might happen automatically when using some build systems and when generating release builds. If you don't want that to happen, maybe calling std::abort() or throwing exceptions might be better.
  • If both the contents of availableTypes and requiredTypes are known at compile-time, then you could use template programming to fill dstVector at compile-time.
  • Consider moving to at least C++17; this adds std::apply() which allows you to iterate over the types contained in a std::tuple. This can be used to avoid a long and repetitive if-else` sequence.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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