Factory Design pattern implementation

[Coding Mage]

Hi all,

I have an interesting problem for which I already have a solution, but I have a feeling my solution is less than elegant.

I need to create a Factory class which returns an instance of some child of class Foo. Which child is instantiated depends on the value of some integer parameter. So, for example, the following would work well enough:

class FooFactory
    {
    public:
       Foo* getFooInstance( int id );
    }

    Foo* FooFactory::getFooInstance( int id )
    {
        switch (id) {
        case 1:
            return new FooBar();
        case 2:
            return new FooBoo();
        default:
            return NULL;
        }
    }

However, I would need to maintain that switch statement everytime I implement a child class of Foo. So, a better solution, I feel, is to register the child class during static initialization:

// FooFactory.h
    class FooFactory
    {
       typedef Foo* (*FooInstantiator)();
       typedef std::map<int, FooInstantiator> InstantiatorMap;

    public:
       Foo* getFooInstance( int id );
       static int registerFooInstantiator(int id, FooInstantiator func);

    private:
       static InstantiatorMap m_instantiatorMap;
    }

    // FooFactory.cpp
    // For the sake of brevity, let's just assume we've guaranteed that
    // m_instantiatorMap already has been constructed.
    int FooFactory::registerFooInstantiator(int id, FooInstantiator func)
    {
        m_instantiatorMap.insert( std::make_pair( id, func ) );
        return 1;
    }

    // FooBar.h
    class FooBar : public Foo
    {
        ... 
        static int isInitialized;
        ...
    }
    Foo* getFooBar();

    // FooBar.cpp
    int FooBar::isInitialized = 
        FooFactory::registerFooInstantiator( 1, getFooBar );

    Foo* getFooBar() { return new FooBar(); }

The above solution allows me to create a new Foo child class and add it to the factory without having to modify the factory code.

Can I do better? Having to create a getFooXXX() function for each child seems unnecessary. I was thinking of creating a template function getFoo<T>, declared in FooFactory.h, where T would be the child class. This removes the need for separate getFooXXX functions. Any thoughts?

[kon_t]

You dont specify how you intend to use the factory. That will play a larg part in choosing a Factory pattern to use. So my comments are fairly general.

The motivation behind Factory, is to put the object creation in one place, making maintenance easier & removing dependencies from 'user' code.
IE you expect to change Factory when new Foo sub classes are added. The change can be adding to the switch, or having a heirarchy of Factory classes.

If the Foo heirarchy is fairly stable, then your switch implementation is great.
(I assume that you have left out error checking for simplicity.)
Even if new Foo classes appear fairly often, the code changes are still limited to one place.

Also, if many Foo objects are created, the switch is faster than the map.

In your implementation, you need a new creation function for each class and you also have to provide code which adds the instantiation function/method to the map in Factory. How will you handle errors (eg a duplicate class id)?

In general, i try to avoid passing ids to the object creation code. It can cause maintennace nightmares down the track in anything beyond small projects.
How does user code know which id relates to which sub class?
What would happen (to user code) if a Foo sub class is removed?

And, on a std c++ note.
It is far better to return the newly created objects as auto_ptrs, as these will automatically handle the deletion for you.

eg
auto_ptr<Foo> FooFactory::getFooInstance

[Chainsaw]

This is sort of a side issue, but I tend to prefer a static method of Foo being the factory, rather than have a seperate class called, in this case, FooFactory. So, the base class would implement

class Foo
{
public:
    <blah blah blah>

    static Foo* Factory();   // or use the auto_ptr<> if you are a fancy 'I don't use old C-style stuff' bit hugger :-)

[kon_t]

This is sort of a side issue, but I tend to prefer a static method of Foo being the factory, rather than have a seperate class called, in this case, FooFactory. So, the base class would implement

class Foo
{
public:
    <blah blah blah>

    static Foo* Factory();   // or use the auto_ptr<> if you are a fancy 'I don't use old C-style stuff' bit hugger :-)

It all depends on what you need.

Your example saves a class (or class hierarchy).
And, it is much simpler than the Factory pattern.
I have used th same thing in quite a few smallish projects.

However it
- doesnt have the flexibility / power of the abstract factory pattern.
- requires that you have complete control of the Foo interface

As to auto_ptr. It isnt a 'I don't use old C-style stuff''.
Using auto_ptr
- allows you to not worry about deleting things (like not having to close a stream obj).
- makes your code more maintainable
- eliminates hard to fix memory leaks when exceptions are thrown.

[klorentzj]

Hi all,

I have an interesting problem for which I already have a solution, but I have a feeling my solution is less than elegant.

I need to create a Factory class which returns an instance of some child of class Foo. Which child is instantiated depends on the value of some integer parameter. So, for example, the following would work well enough:

class FooFactory
    {
    public:
       Foo* getFooInstance( int id );
    }

    Foo* FooFactory::getFooInstance( int id )
    {
        switch (id) {
        case 1:
            return new FooBar();
        case 2:
            return new FooBoo();
        default:
            return NULL;
        }
    }

However, I would need to maintain that switch statement everytime I implement a child class of Foo. So, a better solution, I feel, is to register the child class during static initialization:

// FooFactory.h
    class FooFactory
    {
       typedef Foo* (*FooInstantiator)();
       typedef std::map<int, FooInstantiator> InstantiatorMap;

    public:
       Foo* getFooInstance( int id );
       static int registerFooInstantiator(int id, FooInstantiator func);

    private:
       static InstantiatorMap m_instantiatorMap;
    }

    // FooFactory.cpp
    // For the sake of brevity, let's just assume we've guaranteed that
    // m_instantiatorMap already has been constructed.
    int FooFactory::registerFooInstantiator(int id, FooInstantiator func)
    {
        m_instantiatorMap.insert( std::make_pair( id, func ) );
        return 1;
    }

    // FooBar.h
    class FooBar : public Foo
    {
        ... 
        static int isInitialized;
        ...
    }
    Foo* getFooBar();

    // FooBar.cpp
    int FooBar::isInitialized = 
        FooFactory::registerFooInstantiator( 1, getFooBar );

    Foo* getFooBar() { return new FooBar(); }

The above solution allows me to create a new Foo child class and add it to the factory without having to modify the factory code.

Can I do better? Having to create a getFooXXX() function for each child seems unnecessary. I was thinking of creating a template function getFoo<T>, declared in FooFactory.h, where T would be the child class. This removes the need for separate getFooXXX functions. Any thoughts?

Here's a thought. You can solve the problem of initializing the map with a static initializer like this:

typedef InstantiatorMap::value_type InstantiatorMapEntryType;
InstantiatorMapEntryType FooFactory::InstantiatorMapEntries[] = { 
InstantiatorMapEntryType(  1, &FooBar::GetFooBar ), 
InstantiatorMapEntryType(  2, &FooBar::GetFooBoo ), 
}
InstantiatorMap  FooFactory::m_InstantiatorMap( InstantiatorMapEntries,
InstantiatorMapEntries + sizeof(InstantiatorMapEntries)/sizeof(InstantiatorMapEntries[0]));

This also has the advantage of giving a concise view of which id goes with which function.

You can also make getFooInstance static. Then, you never need to instantiate the FooFactory class.

[Dave Sinkula]

Please avoid thread necrophilia.