Is the typical C++ implementation of Factory class flawed?

Question

I have the need to implement factory class in C++, but when I was thinking about that, I found one big problem that I couldn't solve, and I found out, that all factory implementation examples around are flawed in the same way. I'm probably the one who is wrong, but please tell me why.

So here is simple "typical" factory implementation, it allows me to register new objects without changing the Factory class.

//fruit.h
class Fruit
{
protected :
  int count;
public :
  Fruit(int count) : count(count) {}
  virtual void show() = 0;
};

// factory.h
/** singleton factory */
class Factory
{
  typedef Fruit* (*FruitCreateFunction)(int);
  static Factory* factory;
  std::map<std::string, FruitCreateFunction> registeredFruits;
public :
  static Factory& instance()
  {
    if (factory == NULL)
      factory = new Factory();
    return *factory;
  }
  bool registerFruit(const std::string& name, Fruit* (createFunction)(int))
  {
    registeredFruits.insert(std::make_pair(name, createFunction));
    return true;
  }
  Fruit* createFruit(const std::string& name, int count)
  {
    return registeredFruits[name](count);
  }
};

//factory.cpp
Factory* Factory::factory = NULL;

//apple.h
class Apple : public Fruit
{
  static Fruit* create(int count) { return new Apple(count); }
  Apple(int count) : Fruit(count) {}
  virtual void show() { printf("%d nice apples\n", count); };  
  static bool registered;
};

// apple.cpp
bool Apple::registered = Factory::instance().registerFruit("apple", Apple::create);

//banana.h
class Banana : public Fruit
{
  static Fruit* create(int count) { return new Banana(count); }
  Banana(int count) : Fruit(count) {}
  virtual void show() { printf("%d nice bananas\n", count); };  
  static bool registered;
};

// banana.cpp
bool Banana::registered = Factory::instance().registerFruit("banana", Banana::create);

// main.cpp
int main(void)
{
  std::vector<Fruit*> fruits;
  fruits.push_back(Factory::instance().createFruit("apple", 10));
  fruits.push_back(Factory::instance().createFruit("banana", 7));
  fruits.push_back(Factory::instance().createFruit("apple", 6));
  for (size_t i = 0; i < fruits.size(); i++)
    {
      fruits[i]->show();
      delete fruits[i];
    }
  return 0;
}

Ok, this code looks fancy and it works, but here comes the but:

The C++ standard doesn't allow me to define the order in which global (static) variables will be defined.

I have 3 static variables here

Apple::registered;
Banana::registered;
Factory::factory;

The Factory::factory pointer needs to be defined to NULL before the Apple(or Banana)::registered variable, or the Factory::instance method will work with uninitialized value, and behave unpredictably.

So, what am I not getting here? Is the code really working only by an accident? If so, how should I solve the issue?

Answer 1

All global POD data is guaranteed to be initialized to a constant value before any initializers run.

So at the start of your program, before any of the register calls are made and before main is run, the pointer is NULL and all of the bools are false, automatically. Then the initializers run, including your register calls.

Edit: Specifically, from the standard (3.6.2.2: Initialization of non-local objects):

Together, zero-initialization and constant initialization are called static initialization; all other initialization is dynamic initialization. Static initialization shall be performed before any dynamic initialization takes place.

Answer 2

All static variables are initialized before the program begins to run. They are set at compile time and baked right into the executable.

The only issue arises when one static variable depends on another:

In a.hpp:

static int a = 1;

in b.hpp:

extern int a;
static int b = a;

The order in which static variables are initialized is not well defined, so b may or may not be 1 in this example. As long as your variables don't depend on each other, you're fine. Furthermore, is you don't give an initial value, static members are set to zero by default.

Answer 3

I've tended to see the 'instance' method of Factory implemented as follows:

static Factory& instance()
{
    static Factory *factory = new Factory();
    return *factory;
}

However, the point is that all access to the instance runs through the static instance method. The calls to register the two fruit classes for example use Factory::instance() to obtain the singleton which will guarantee that the initializer for Factory::factory has executed. In my posted alternative implementation the static initialization only occurs the first time the method is called.

The possible issues with Apple::registered and Banana::registered depend on where they might be used from. In the posted code they aren't used at all. If used only within apple.cpp and banana.cpp respectively then there is no issue with order of initialization.