How to implent Singleton Pattern and Factory Pattern in C++11 code

Question

I'm trying to implement the Singleton Pattern and Factory Pattern at the same time. In my mind there can be only one factory for each kind of factory which are based on an abstract class, and use the factory to produce the corresponding shape object.

But I got an error:

error LNK2001: unresolved external symbol "private: static class TriangleFactory * TriangleFactory::instance" (?instance@TriangleFactory@@0PAV1@A)

I don't know why this happens.Could you tell me how to fix it? The code is as follows.

#include<type_traits>
#include<iostream>
#include<cstdlib>
#include<cstdio>
#include<memory>
#include<time.h>
using namespace std;

class Shape
{
public:
    virtual void display();
protected:
    int x, y;
};

class MyTriangle:public Shape
{
public:
    MyTriangle(int _x, int _y)
    {
        x = _x;
        y = _y;
    }
    MyTriangle()
    {
        x = rand() % 200;
        y = rand() % 200;
    }
    void display()
    {
        printf("%d %d\n",x,y);
    }
};

class SuperFactory
{
public:
    virtual Shape* Execute() = 0;
};

class TriangleFactory :public SuperFactory
{
public:
    Shape* Execute()
    {
        Shape *p = new MyTriangle;
        return p;
    }
    static SuperFactory* GetFactory()
    {
        return instance;
    }
private:
    TriangleFactory()
    {
        instance = nullptr;
        instance = new TriangleFactory;
    }
    ~TriangleFactory(){}
    static TriangleFactory* instance;
};

int main()
{
    MyTriangle *p = dynamic_cast<MyTriangle*>(TriangleFactory::GetFactory()->Execute());
    p->display();
    return 0;
}

would appreciate your answers :)

Answer 1

My criticisms:

1. You don't need all those #includes. You can get away with only <iostream>.

2. Your constructors and deconstructors are private. I suggest that you: a.) make them protected instead, and b.) only make the constructor private. The deconstructor should be public and virtual.

3. The function Shape::display should be declared as a pure virtual function like SuperFactory::Execute. Change it to this: virtual void display() = 0;

4. The main problem with the other answers - simply writing TriangleFactory* TriangleFactory::instance; before your main() function is this: you'll find yourself facing a lovely null pointer error. The solution to this requires a change in your singleton. Because you initialize instance in your constructor, and no object of type TriangleFactory is ever created in the code you posted, the constructor is never called, and instance is never initialized.

My solution:

Continue with the TriangleFactory* TriangleFactory::instance; before the main() function, but set its value to null like this TriangleFactory* TriangleFactory::instance = nullptr;. Doing so avoids errors relating to past memory leaks.

Instead of initializing instance in your constructor, leave the constructor empty and change your GetFactory function to this:

static SuperFactory *GetFactory()
{
    if(!instance)
    {
        instance = new TriangleFactory();
        return instance;
    }
    else
    {   
        return instance;
    }
}

Now, why would I change all that when putting TriangleFactory *TriangleFactory::instance = new TriangleFactory(); before main() - as I suspect you did - works just fine? Well, initializing any data outside of a function is risky. The objects are created in an arbitrary order at runtime - and when you start creating classes that rely on other classes, TriangleFactory *TriangleFactory::instance = new TriangleFactory(); will blow up in your face. Don't have stuff blow up in your face. Get rid of cable and upgrade to DirecTV. No xD just kidding, but definitely don't do TriangleFactory *TriangleFactory::instance = new TriangleFactory();

Answer 2

Correct way in c++ to initilize singleton in your case would be to use static instance, not static pointer. This way the object will be properly destroyed on program exit.

So the correct look of GetFactory() would be:

static SuperFactory * GetFactory(){

    static TriangleFactory factory;
    return &factory;
}

The factory instance will be created on first use of GetFactory() and destroyed on program exit. No memory leak and the most simplest solution.

Answer 3

The line:

static TriangleFactory* instance; 

Declares instance. There is no definition. Add:

TriangleFactory* TriangleFactory::instance;

This will define the symbol for linker.

Answer 4

You need to input below line.

TriangleFactory* TriangleFactory::instance;

The error message is telling you that the compiler cannot find the definition instance. You need to init the static member variable, instance, when you use it as class member variable.