1
\$\begingroup\$

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

\$\endgroup\$
4
  • 1
    \$\begingroup\$ The factory function you want is usually std::make_unique, from the Standard Library. It accepts any and all valid constructor arguments, and you can assign it to a std::shared_ptr, or any kind of smart pointer to a base-class interface for polymorphism. Without more information, I can’t tell you if that meets your requirements, but you should always start by asking if it does. \$\endgroup\$
    – Davislor
    May 27 at 19:59
  • \$\begingroup\$ I will had more information to the question \$\endgroup\$ May 28 at 10:56
  • 1
    \$\begingroup\$ You can use std::shared_ptr<Base> polymorphic = std::make_unique<Derived>(foo, bar); Or std::make_shared would work too. \$\endgroup\$
    – Davislor
    May 28 at 11:51
  • \$\begingroup\$ Please do not edit the question, especially the code, after an answer has been posted. Changing the question may cause answer invalidation. Everyone needs to be able to see what the reviewer was referring to. What to do after the question has been answered. \$\endgroup\$
    – pacmaninbw
    May 28 at 13:06

1 Answer 1

0
\$\begingroup\$

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.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ I would normally use std::make_unique, which converts to a std::shared_ptr (but not vice versa). \$\endgroup\$
    – Davislor
    May 28 at 11:49
  • \$\begingroup\$ How can I make the factory type safe? \$\endgroup\$ May 28 at 14:12
  • \$\begingroup\$ You can make registry_map a static inline template. But why would you want to if you can just use std::make_shared() as I've shown in the first section? \$\endgroup\$
    – G. Sliepen
    May 28 at 15:35
  • \$\begingroup\$ because I would like to use this as a factory method for other classes which can have multiple signatures for constructors. This way my factory knows how to construct an object in several ways. However, it was pointed out to me that using reinterpret_cast is not safe. I would like for the code be as safe and robust as possible \$\endgroup\$ May 28 at 15:42

Your Answer

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

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