current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I am primarily a java programmer and for the first time working in C++.

I have implemented a Factory design pattern in C++.

    class TaskFactory
    {
    public:
        typedef Task* (*create_callback)(map<string, string> vMap); 
        static void register_task(const string& type, create_callback cb); // function for registering the classes with Factory
        static void unregister_task(const string& type); // function for unregistering the classess with factory
        static Task* create_task(const string& type,map<string, string> vMap); // function for creating instance of Tasks classes on the basis of class type
    private:
        static map<string, create_callback> mTasks; // map to store class types along with callback
    };

Following is the factory implementation.

    map<string, TaskFactory::create_callback> TaskFactory::mTasks;

    //registers the task with the factory
    void TaskFactory::register_task(const string& type, TaskFactory::create_callback cb){
        mTasks[type]= cb;
    }
    //unregisters the task with the factory
    void TaskFactory::unregister_task(const string& type){
        mTasks.erase(type);
    }
    //creates instance of Task subclasses on the basis of task
    Task* TaskFactory::create_task(const std::string& type, map<string, string> vMap){
        map<string, TaskFactory::create_callback>::iterator it=mTasks.find(type);
        if(it==mTasks.end()){
            return NULL;
        }else{
            return mTasks[type](vMap);
        }
    }

And my base Task class is below:

    class Task{
    private:
        map<string, string> vMap;
        Task(map<string, string>); 
    public:
                    static Task* create(map<string, string>); // funtion is used for creating instance of the Task
            };

In code review I was told that Factory implementation is not doing its intended job. But in my opinion its working very well. Is any thing wrong with this implementation?

share|improve this question
add comment

1 Answer

up vote 4 down vote

Few small problems with the factory.

You are using a pointer to a method.

        typedef Task* (*create_callback)(map<string, string> vMap);

As the way of creating your object. This is very C like. It also introduces the possibilities of passing a NULL pointer (which is not good).

The more idiomatic way to to declare a functor:

struct TaskCreator
{
    // Leaving Task* for similarities to current code (I will get back to it).
    Task* operator()(std::map<std::string, std::string> vMap) const
    {
        return build(vMap);
    }
    private:
        virtual Task* build(std::map<std::string, std::string> vMap) const =  0;
};

Register objects

Now you should register objects derived from this class with the factory. This will prevent you accidentally passing NULL as a callback.

// This looks incant because there are no `*` here
// But there is a hidden '*' in create_callback 
static void register_task(const string& type, create_callback cb);

// I would change this too:
// Notice I am not using `static` here. I will get to that in a second.
void register_task(const string& type, TaskCreator& cb);
                                          //     ^^^  That is important.
// Alternatively you can register via smart pointers.
// But I prefer to pass references. Then make the `TaskCreator` automatically
// Register themselves in their constructor.

Order of construction

You also have another invisible problem in the order of construction across compilation units. Remember that the order of construction of static storage duration objects is undefined across compilation units.

static map<string, create_callback> mTasks;

This object is constructed before main() is called. The problem is you do not know exactly when. As a result if you call register_task() before main() you don't know if it will work because the storage mTasks may not have been created yet.

There are a couple of ways of solving this. The easiest to use a static member of a function as the storage.

static map<string, create_callback>& getTaskStorage()
{
    static map<string, create_callback> mTasks;
    return mTasks;
}

But I would go the other way. And use a factory object pattern.

Factory object

You are using a set of static methods as your factory. I would swing this around and actually create a factory object. The problem with static functions is that it is hard to use test code with them. It is much easier to mock an object than static API.

Ownership

Last. You are not expressing ownership semantics for the created object.

static Task* create_task(const string& type,map<string, string> vMap);

A pointer Task* is totally useless in this situation as you have no idea if the ownership is being retained by the factory (and it will destroy it) or if ownership is being returned to the caller so he can destroy it.

Here you have two options:

  • Return a reference.
    • This indicates that the factory is retaining ownership of the created object. This means the object will be destroyed when the factory is destroyed (This implies a factory object).
  • Return a smart pointer
    • By returning a smart pointer you are indicating somthing else has ownership. The type of ownersip depends on the type of smart pointer (unique_ptr: ownership is returned to the caller, shared_ptr: ownership is shared and the loss of the last reference will delelte it).

Further Advice.

I would suggest some more specific recommendations. But unfortunately you do not provide the expected usage semantics of your factory. Thus I may write something that is tially usless. If you can write some more details on how you think the factory is going to be used I can give you some more concrete information.

Errors in the code:

class Task
{
     static Task* create(map<string, string>);
};

If you take this address it does not match:

typedef Task* (*create_callback)(map<string, string> vMap);

There is nothing in the standard that guarantees that the calling convention of a normal function matches the calling convention of static methods. It just happens to work on your compiler but it is non portable.

As stated above: What happens of somebody registers a NULL function.

return mTasks[type](vMap);

This line is likely to fail. Using your method you need to validate that the pointer is not NULL wither at the point of setting or at the point were it is used.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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