Factory pattern with variadic constructor

Question

I have this code for a factory design pattern that will take into account any number of arguments in the constructor. Through a static boolean in the class a certain constructor is registered in the factory. However it requires reinterpret_cast to run. Also the boolean strategy is not very elegant (it works, but If I have 4 constructors I will have to have 4 booleans to perform the registry).

I would like to have a robust approach to register the appropriate constructor.

How can I improve the code?

#include <unordered_map>
#include <string>
#include <iostream>
#include <memory>
 
class Base;

class myFactory
{
public:
    typedef std::unordered_map<std::string, void*(*)()> registry_map;
 
    virtual ~myFactory() = default;
 
    static registry_map & registry()
    {
        static registry_map impl;
        return impl;
    }

    template<typename ...Ts>
    static std::shared_ptr<Base> instantiate(std::string const & name, const Ts&...args)
    {
        auto it = registry().find(name);
        if ( it == registry().end()) return nullptr;
        typedef std::shared_ptr<Base> (*create_type)(Ts...);
        auto create_fun = reinterpret_cast<create_type>(it->second);
        return create_fun(args...);
    }

    template<typename F>
    static bool sign(const std::string& name, F* func)
    {
        registry()[name] = reinterpret_cast<void*(*)()>(func);
        return true;
    }
};
 
class Base: public myFactory
{
    public:
        Base(int a):var1(a){}
        Base(int a, int b):var1(a+b){}
        virtual ~Base() = default;
        virtual void f() = 0;
        int var(){return var1;}
    
    private:
        int var1;
};
 
class DerivedExample : public Base
{
private:
    static bool sign1;
    static bool sign2;

public:
    DerivedExample(int a):Base(a){std::cout << var()<< std::endl;}
    DerivedExample(int a, int b):Base(a,b){std::cout << a << b << std::endl;}
    
    static std::shared_ptr<Base> create(int a) { return std::make_shared<DerivedExample>(a);}
    static std::shared_ptr<Base> create(int a, int b) { return std::make_shared<DerivedExample>(a,b);}

    virtual void f() override { std::cout << "DerivedExample" << std::endl; }
};

bool DerivedExample::sign1 = DerivedExample::myFactory::sign("DerivedExample1", static_cast<std::shared_ptr<Base> (*)(int)>(&DerivedExample::create));
bool DerivedExample::sign2 = DerivedExample::myFactory::sign("DerivedExample1", static_cast<std::shared_ptr<Base> (*)(int, int)>(&DerivedExample::create));


int main()
{
    std::shared_ptr<Base> p1 = Base::instantiate("DerivedExample1", 10);
    std::shared_ptr<Base> p2 = Base::instantiate("DerivedExample1", 1, 2);
    p1->f();
    p2->f();
    return 0;
}

Kind regards

Answer 1

Just use std::make_shared()

You can achieve the same results with std::make_shared() alone. Consider that the constructors of std::shared_ptr and std::unique_ptr allow converting from a smart-pointer-to-derived to a smart-pointer-to-base. So you can write:

std::shared_ptr<Base> p1 = std::make_shared<DerivedExample>(10);
std::shared_ptr<Base> p2 = std::make_shared<DerivedExample>(1, 2);

As Davislor mentioned, you can also use std::make_unique to create a std::unique_ptr, and then later store that in a std::shared_ptr if so desired.

Your factory is not type-safe

With your code, the following lines compile without any errors:

std::shared_ptr<Base> p1 = Base::instantiate("DerivedExample1");
std::shared_ptr<Base> p2 = Base::instantiate("DerivedExample1", 1, 2, 3);

How can that be? Well first of all, even though you have two calls to sign(), there is only one entry in myFactory::registry_map with the key "DerivedExample1", and it points to the function that is supposed to take two arguments. However, you erased the type of the function pointer when you stored it in registry_map. So instantiate() has no way of checking that the function it finds in the map actually has the right number of arguments. It just assumes it is correct.

Because of this, your factory is not safe: it is easy to make a mistake, which the compiler doesn't catch, and which might result in unexpected behavior at runtime. The construction of p1 in your example already results in undefined behavior.

To make it type-safe, you should not erase the type of the function you store in the map. You can do this by making the function registry itself templated on the function type:

template<typename F>
static auto& registry()
{
    static std::unordered_map<std::string, F*> impl;
    return impl;
}

template<typename F>
static void sign(const std::string& name, F* func)
{
    registry<F>()[name] = func;
}

It becomes a bit more tricky to instantiate though; naively you would write:

template<typename ...Ts>
static auto instantiate(std::string const & name, const Ts&...args)
{
    using F = std::shared_ptr<Base>(Ts...);
    auto it = registry<F>().find(name);
    …
}

But this assumes, just like you did in your code, that all the arguments are passed by value. What if they are passed by l-value or r-value reference? What if I want to call it with parameters of types that are convertible to Ts, but not Ts themselves? For example:

auto foo = DerivedExample::create(3.1415); // implicitly converted to int with value 3

But if I would write:

auto bar = Base::instantiate("DerivedExample1", 3.1415); // oops?

With your code that would result in undefined behavior due to its type-unsafety, with my example above it would fail to find the desired function in the registry. Again, just using std::make_shared() would do the right thing (or at least, the same as calling create() directly):

std::shared_ptr<Base> baz = std::make_shared<DerivedExample>(3.1415);

Write one statement per line

Some of your lines contain multiple statements. This makes the lines very long, so part of it is not visible unless you have a very wide window. Prefer writing one statement per line, like:

DerivedExample(int a): Base(a) {
    std::cout << var() << '\n';
}

I recommend however that you do not manually change your code style, instead use a code formatting tool, either the one in your code editor if it has one, or an external one like Artistic Style or ClangFormat.

Use '\n' instead of std::endl

Use '\n' instead of std::endl; the latter is equivalent to the former, but also forces the output to be flushed, which is usually unnecessary and might hurt performance.