Simple factory retrieving object by name [closed]

Question

Despite the fact that this code works, I would like to know if there's any situation I did not anticipated, and/or if what I am trying to do is some what wrong.

-std=C++11 flag needed

#include <iostream>
#include <string>
#include <map>
#include <typeinfo>
#include <functional>

class object
{
    public:

    virtual ~object(){}

    virtual std::string to_string() = 0;
};

class SomeObject : public object
{
    public:

    SomeObject(){}
    virtual ~SomeObject(){}

    virtual std::string to_string()
    {
        return "I am a type of SomeObject";
    } 
};

class SomeOtherObject : public object
{
    public:

    SomeOtherObject(){}
    virtual ~SomeOtherObject(){}

    virtual std::string to_string()
    {
        return "I am a type of SomeOtherObject";
    } 
};

std::map<std::string, std::function<object*()> > types;

template<typename O>
inline
static void register_type(const std::string & name)
{
    types[name] = [](){ return new O; };
}

static object * get_object(const std::string & object_name )
{
    return types[object_name]();
}

int main()
{
    register_type<SomeObject>("SomeObject");
    register_type<SomeOtherObject>("SomeOtherObject");

    object * some = get_object("SomeObject");
    object * some_other = get_object("SomeOtherObject");

    std::cout << "::" << some->to_string() << std::endl;
    std::cout << "::" << some_other->to_string() << std::endl;

    delete some;
    delete some_other;

    std::cout << "exit" << std::endl;

    return 0;
}

Answer 1

In general your code looks fine, though there are some points to improve:

Memory management

Instead of using raw pointers and new delete, you should use a smart pointer to express transfer of ownership explicitly:

template<typename O>
void register_type(const std::string & name) {
    types[name] = [](){ return std::unique_ptr<O>(new O); };
}

std::unique_ptr<object> get_object(const std::string & object_name ) {
    return types[object_name]();
}

Thus it's clear, the caller will own the object instance, and doesn't need to struggle calling delete correctly if the obtained instance goes out of scope.

Template parameter naming

The more usual name for the template parameter is rather T than O

Use a factory class instead of global functions and global map

Consolidate the code of the register_type(), get_object() functions and the types map variable into a class Registry, and provide a single instance of that one.

This would allow to encapsulate the types map as a private class member.

You could consider making that class a Singleton.

Check if the type name is registered

You should check if the string passed to the get_object() function was already registered in the map, rather than blindly calling the default (NOP) function from the map.

Usage of static for a global function

You should be aware that static actually make the register_type() function only visible for the translation unit, it appears in. Actually you should not need it.

---

BTW, the technique you're using is often seen for unit testing frameworks (like e.g. google-test) as test case factories, also often coming along with stringizing macros for the registered classes.

Answer 2

• You don't need that space in

  std::map<std::string, std::function<object*()> > types;
                                                ^
  

  anymore. C++11 has learned that >> in this case is not the shift operator.

• Why is register_type static and inline? I don't think either one makes sense here.

• Using T as the template parameter type is very common, O not so much, making the code harder to read.

• Returning a raw owning pointer is bad style in C++11. Return a std::unique_ptr instead, since it will prevent a leak by default. If people don't like it they can call .release(), so you are not losing flexibility.

• Some people consider std::endl bad style since it flushes the output, needlessly slowing down the program.

• Maybe you could extend register_type to accept types that are not default constructible.

Answer 3

Remove unnecessary virtual functions

In your SomeObject and SomeOtherObject classes, are you expecting them to be inherited from as well? If not, then remove the virtual and use the override contextual keyword from C++11 instead. This will allow you to make your intent of overriding the toString() function explicit as well as provide a compile-time check for whether or not you're actually overriding the correct function. This might be overkill in this specific example (due to its simplicity) but it's a good habit to get into. You can also remove your unnecessary definition of the default ctors and dtors for these classes (as the compiler provides them automatically for you).

class object
{
    public:

    virtual ~object(){}

    virtual std::string to_string() = 0;
};

class SomeObject : public object
{
    public:

    std::string to_string() override
    {
        return "I am a type of SomeObject";
    } 
};

class SomeOtherObject : public object
{
    public:

    std::string to_string() override
    {
        return "I am a type of SomeOtherObject";
    } 
};

Use a Factory Registry Class / Memory Management

As others have mentioned, turn your global map and registry/unregister functions into a class that can manage its own state. Moreover, use std::unique_ptr instead of operator new here.

Replace std::map with std::unordered_map

It looks like you don't need the ordering (and consequently the log(n) insertion time) that std::map gives you. If you're looking for a hash table (amortized O(1) insertion/retrieval), then use std::unordered_map.

Extending register_type() for non default constructable objects

Use variadic templates for this:

struct Registry
{

    template <class T, class... Args>
    static void register_type(const std::string &name, Args&&... args)
    {
        types[name] = [&args...]() { return std::make_unique<T>(std::forward<Args>(args)...); };
    }

    static std::unique_ptr<object> get_object(const std::string &name)
    {
        return types[name]();
    }

private:
    static std::unordered_map<std::string, std::function<std::unique_ptr<object>()>> types;
};

Answer 4

Nearly everything is already handled in the other answers, but anyway:

1. Indenting the access-specifier public: to the same level as the members is uncommon, you want it to stand out:

   Indent it to the same level as the containing class.

2. If you want the default implementation for a special member function (default/copy/move constructor, copy/move assignment, destructor), prefer implicit declaration over explicit defaulting = default; in-class, over explicit defaulting outside the class over providing it yourself.

   Only if it is not user-provided can it be trivial, which can be detected and is optimized for.

   (Applying this means that none of your example-classes retain any explicitly declared member but to_string).

3. Consider adding a namespace-scope to_string, to follow the pattern of std::to_string.

4. Declaring a free function both static and inline, while not an error, is certainly futile:

   • static means it is translation-unit (TU) local.
   • inline means it is not an error if the function is provided by multiple TUs, as long as it's identical in every instance.

5. return 0; is implicit for main.