6
\$\begingroup\$

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;
}
\$\endgroup\$

closed as off-topic by Jamal Feb 12 '16 at 1:05

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ Are these the original variable names? They make this code look hypothetical. \$\endgroup\$ – Jamal Dec 20 '15 at 23:38
  • \$\begingroup\$ @Jamal They're probably not. But the technique described works, and can be reviewed from an abstract POV, or not? \$\endgroup\$ – πάντα ῥεῖ Dec 20 '15 at 23:48
  • \$\begingroup\$ @Jamal I just made this code and compiled with a comipler in a website. I think I can't answer your question properly. \$\endgroup\$ – Rafael Lucio Dec 20 '15 at 23:49
  • \$\begingroup\$ I've always had reservations about Simple Factory from a security standpoint. Someone (in client code) can create her own concrete class, register it, then have it executed somewhere by just injecting its name... Maybe a lot of ifs, but this seems riskier than static class names. \$\endgroup\$ – Fuhrmanator Dec 21 '15 at 21:53
  • \$\begingroup\$ but thats my point of using it \$\endgroup\$ – Rafael Lucio Dec 21 '15 at 23:58
6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I was "hoping" that the idea in general was wrong because I use a template parameter inside a lambda to return a pointer outside compile time. But if it's not the case, I am glad. And I really appreciate your effort and you made me a better programmer. Thank you. \$\endgroup\$ – Rafael Lucio Dec 20 '15 at 23:01
4
\$\begingroup\$
  • 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.

\$\endgroup\$
1
\$\begingroup\$

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;
};
\$\endgroup\$
  • \$\begingroup\$ Not using virtual does not have any effect on the classes, as the affected members are overriding inherited virtual functions. It duplicates part of the info provided by override without adding anything new though, which is not good. \$\endgroup\$ – Deduplicator Dec 23 '15 at 18:06
  • \$\begingroup\$ Ah, I was under the impression that adding virtual in a derived class when it wasn't needed would generate an unnecessary vtable. I wonder where I got that from? Anyways, override does that better now and provides a compile time check :) I'll edit the answer. \$\endgroup\$ – Bizkit Dec 23 '15 at 18:18
0
\$\begingroup\$

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.

\$\endgroup\$

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