3
\$\begingroup\$

I created a template registry class. It supports custom factory functions and custom constructors. Any kind of feedback is welcome (style, performance, bugs)

registry.hpp:

#ifndef REGISTRY_HPP
#define REGISTRY_HPP

//STL
#include <unordered_map>
#include <memory>

template<typename Key>
class RegistryConstructionError : public std::exception
{
public:
    RegistryConstructionError(const Key &key)   :key(key){}

    const char *what()const throw(){return "Undefined key";}
    const Key key;
};

template<typename Base, typename Key, typename... Args>
class Registry
{
public:
    typedef std::unique_ptr<Base> (*FactoryFunc)(Args... args);

    Registry() = default;
    ~Registry() = default;

    template<typename Derived>
    void registerClass(const Key &key);
    void registerFactory(const Key &key, const FactoryFunc factory);

    std::unique_ptr<Base> construct(const Key &key, Args... args);

private:
    template<typename Derived>
    static std::unique_ptr<Base> constructDerived(Args... args);

    std::unordered_map<Key, FactoryFunc> m_map;

public:
    typedef RegistryConstructionError<Key> ConstrError;
};

#include "registry.tpp"

#endif // REGISTRY_HPP

registry.tpp:

template<typename Base,
        typename Key,
        typename... Args>
template<typename Derived>
void Registry<Base, Key, Args...>::registerClass(const Key &key)
{
    m_map[key] = &constructDerived<Derived>;
}

template<typename Base,
        typename Key,
        typename... Args>
void Registry<Base, Key, Args...>::registerFactory(const Key &key, const FactoryFunc factory)
{
    m_map[key] = factory;
}

template<typename Base,
        typename Key,
        typename... Args>
std::unique_ptr<Base> Registry<Base, Key, Args...>::construct(const Key &key, Args... args)
{
    try{
        return m_map.at(key)(std::forward<Args>(args)...);
    }
    catch(const std::out_of_range &e)
    {
        throw ConstrError(key);
    }
}

template<typename Base,
        typename Key,
        typename... Args>
template<typename Derived>
std::unique_ptr<Base> Registry<Base, Key, Args...>::constructDerived(Args... args)
{
    return std::make_unique<Derived>(std::forward<Args>(args)...);
}
\$\endgroup\$
  • \$\begingroup\$ Can you give us an example how you are using it \$\endgroup\$ – Bob5421 Jan 21 at 22:08
2
\$\begingroup\$

Good stuff!

I definitely see a few improvements you could make here:

Prefer using std::function to function pointers

There is a tiny bit more overhead invovled, but the extra flexibility is well worth it.

using FactoryFunc = std::function<std::unique_ptr<Base>(Args...)>;

Another advantage is that it lets you get rid of constructDerived entirely, and replace it with a nice lambda in registerClass.

Nitpick: registerClass should invoke registerFactory.

Since registerClass is just a specialization of registerFactory, it should invoke it instead of reimplementing it. This is preferable because having a single point of entry makes refactorings like adding logging or debug monitoring a lot less error-prone.

Prefer using instead of typedef

The semantics of type alisases are just a lot more legible that way.

Wrong function definition for forwarding args

If you want to use forwarding as you do, construct needs to be declared in a way to use universal references, like so:

template<typename... CtrArgs>
std::unique_ptr<Base> construct(const Key &key, CtrArgs&&... args)

Otherwise, your std::forward<> call is useless and might as well be a std::move.

Personal opinion: The custom exception type is not necessary here.

Letting the out_of_range exception bubble up is perfectly acceptable here, and lets your code be cleaner.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the great response! I implemented most of your points now. Capturing values in factories is very neat - didn't think of that. \$\endgroup\$ – HenrikS Dec 2 '17 at 11:16

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.