C++ Abstract Factory using templates

Question

I'm trying to create an abstract factory template for multiple abstract factories in C++ and came up with this.

#define _CRTDBG_MAP_ALLOC
#include <crtdbg.h>
#include <map>
#include <stdio.h>

class Base
{
public:
    virtual ~Base() {}

    virtual bool Get() = 0;
};

class DerivedA : public Base
{
public:
    bool Get()
    {
        return true;
    }
};

class DerivedB : public Base
{
public:
    bool Get()
    {
        return false;
    }
};

template <class T>
class Creator
{
public:
    virtual ~Creator(){}
    virtual T* Create() = 0;
};

template <class T>
class DerivedCreator : public Creator<T>
{
public:
    T* Create()
    {
        return new T;
    }
};

template <class T, class Key>
class Factory
{
public:
    void Register(Key Id, Creator<T>* Fn)
    {
        FunctionMap[Id] = Fn;
    }

    T* Create(Key Id)
    {
        return FunctionMap[Id]->Create();
    }

    ~Factory()
    {
        std::map<Key, Creator<T>*>::iterator i = FunctionMap.begin();
        while (i != FunctionMap.end())
        {
            delete (*i).second;
            ++i;
        }
    }
private:
    std::map<Key, Creator<T>*> FunctionMap;
};

int main(int argc, char** argv[])
{
    _CrtSetDbgFlag(_CrtSetDbgFlag(_CRTDBG_REPORT_FLAG) | _CRTDBG_LEAK_CHECK_DF);

    //Register
    Factory<Base, char*> temp;
    temp.Register("DA", (Creator<Base>*)new DerivedCreator<DerivedA>);
    temp.Register("DB", (Creator<Base>*)new DerivedCreator<DerivedB>);

    //Pointer to base interface
    Base* pBase = 0;

    //Create and call
    pBase = temp.Create("DA");
    printf("DerivedA %u\n", pBase->Get());
    delete pBase;

    //Create and call
    pBase = temp.Create("DB");
    printf("DerivedB %u\n", pBase->Get());
    delete pBase;

 return 0;
}

It compiles and runs fine with no memory leaks (win32 crtdbg) but I don't know if this is really the correct way to do an abstract factory template.

temp.Register("DA", (Creator<Base>*)new DerivedCreator<DerivedA>);

I'm also wondering about the line above. I'm confused why I have to cast. I don't understand templates very well but I'd assume that it should work fine considering that both the template class and the actual class are derived.

That code actually works fine as shown above and even deletes fine with no memory leaks. I just don't feel entirely comfortable with it.

I haven't been able to find any real examples of template classes except for this from MaNGOS (wow emulator) - https://mangos.svn.sourceforge.net/svnroot/mangos/trunk/src/framework/Dynamic/ObjectRegistry.h

But I don't think I can use that method in my project because I plan on using DLLs at some point in my project and it uses CRTP which is against my requirement for runtime polymorphism.

Transcendentalism

Yep, the line you posted is bad. There is no relation between the two types. They are specialized for different types. I'm also not sure why you bother with CRTP at all. Usually, it is used to avoid virtual functions. But you still have those, so why bother with the templates? – Hanes

Well what I'm trying to do is create a 3 part solution. Program, Library, and DLL. The DLL will contain the implementation, The Library contains the factory, and the program uses the interface. The template exists because I will be doing this alot. I'm using it to replace my current game engine's driver selection. Currently it has copy/pasted code for Video, Physics, Input, and Audio. – Transcendentalism

Answer 1

The class DerivedCreator<DerivedA> is a Creator<DerivedA> not a Creator<Base>.

You need to tell the derived template what the base type so it can implement the interface of Creator<Base> by creating an instance of the derived type:

// DerivedCreator is Creator<BaseType> which creates a 
// DerivedType, not a Creator<DerivedType>
template <class DerivedType, class BaseType>
class DerivedCreator : public Creator<BaseType>
{
public:
    BaseType* Create()
    {
        return new DerivedType;
    }
};

// Register
Factory<Base, std::string> temp;
temp.Register("DA", new DerivedCreator<DerivedA, Base>);
temp.Register("DB", new DerivedCreator<DerivedB, Base>);

// or if you want to create lots with the same base:
template <class DerivedType>
class DerivedBaseCreator : public DerivedCreator<DerivedType, Base> {};

//Register
Factory<Base, std::string> temp;
temp.Register("DA", new DerivedBaseCreator<DerivedA>);
temp.Register("DB", new DerivedBaseCreator<DerivedB>);

Barcot

Ah thank you I think this is exactly what I was looking for. On my own I didn't understand that "The class DerivedCreator<DerivedA> is a Creator<DerivedA> not a Creator<Base>.". – Transcendentalism

Answer 2

Small remarks to improve the design : 1) Use shared_ptr instead of raw pointers 2) use std::string instead of char*

You have to cast, because types Creator, Creator and Creator< DerivedB > are completely different types. The way to fix that is to remove casts :

//Register
Factory<Base, char*> temp;
temp.Register("DA", new DerivedCreator<Base>);
temp.Register("DB", new DerivedCreator<Base>);

Michale

Blindly substituting raw pointers for shared_ptr is a pretty bad idea. Not only do you risk getting circular references, and the associated memory leaks, it's also by far the slowest kind of smart pointer. Use smart pointers, yes. But only use shared_ptr specifically when you absolutely need shared ownership. – Hanes

Yes I definitly agree that moving away from char* is a good idea. Found that one out the hard way when I called pBase = temp.Create("DA"); from a separate address space than the register function. I do plan on using smart pointers but I'm better at simple programming without them so I will add them later. – Transcendentalism

Answer 3

You can make Factory::Register a template method and use boost mpl assert inside

#include <boost/mpl/assert.hpp>
#include <boost/type_traits.hpp>

template <class T, class Key>
class Factory
{
public:
///////////////////////////////////
template <typename _Base>
void Register(Key Id, Creator<_Base> * Fn)
{
    BOOST_MPL_ASSERT((boost::is_base_of<T, _Base>));

    FunctionMap[Id] = reinterpret_cast<Creator<T>*>(Fn);
}
///////////////////////////////////
//...
};

Reginareginald

The only thing you can portably do with reinterpret_cast is cast the result back to the original type, so to use FunctionMap you would have to know the specific types of the creator you get from it, which is not a viable option. – Barcot

You dont have to cast it back because assert guarantees that _Base is derived from (or is) T type, compiler makes sure Fn points to Creator<"whatever"> type. – Reginareginald