Factory Method Pattern - Is this example correct?

Question

Can someone please confirm if below example has properly implemented Factory Method design pattern? If not then please suggest necessary modifications.

#include<iostream>

using namespace std;

class Stooge
{
    public:
        virtual void slap_stick() = 0;
};

class Larry: public Stooge
{
    public:
        void slap_stick()
        {
            cout << "Larry: poke eyes\n";
        }
};

class Moe: public Stooge
{
    public:
        void slap_stick()
        {
            cout << "Moe: slap head\n";
        }
};

class Curly: public Stooge
{
    public:
        void slap_stick()
        {
            cout << "Curly: suffer abuse\n";
        }
};

class stoogeFactory
{
    public:
    static Stooge * make_stooge(int choice);       
};

Stooge * stoogeFactory :: make_stooge(int choice)
{
    switch (choice)
    {
        case 1:
        return new Larry;
    case 2:
        return new Moe;
    case 3:
        return new Curly;
    default:
        cout<<"Invalid choice. Please try again."<<endl;
        return NULL;
    }
}

int main()
{
    Stooge * myStooge;
    int choice;
    while (true)
    {
        cout << "Larry(1) Moe(2) Curly(3) Exit(0): ";
        cin >> choice;

        if (choice == 0)
            break;

        myStooge=stoogeFactory::make_stooge(choice);    
        if(myStooge!=NULL)
        {
        myStooge->slap_stick();
        delete myStooge;
        myStooge=NULL;
        }
    }
    return 0;
}

In above code, I have created an abstract class called Stooge and 3 concrete classes Larry, Moe & Curly. The stoogeFactory class has a static function called make_stooge to accept the user choice and creates the required stooge.

This example make sure that the client should not necessarily know the all the concrete stooge classes available. Addition of new stooge class is also flexible. It also moves the lot of scattered new keyword out of client code.

Please suggest.

Thank You.

Please note the main concern i have is does this example correctly implement Factory Method. I see in GOF that Factory Method structure has four component namely abstract product class, concrete product class, abstract creator class and concrete creator class. Also, Creating objects of concrete product class is the responsibility of concrete creator class and not abstract creator class.

In my above example, i do not have concrete creator class as i don't find it necessary to have. Hence i need to know is this example correct?

Please suggest. Thanks.

Answer 1

1. instead returning a raw pointer, return a smart pointer. I would suggest std::unique_ptr<Stooge>
2. instead of returning NULL, throw an exception. If you return NULL, you have to check if the creation was successful.
3. the make_stooge method doesn't have to be static
4. make Stooge's destructor virtual

---

This compiles fine, and valgrind reports no problems :

#include<iostream>
#include<memory>

using namespace std;

class Stooge
{
    public:
        virtual ~Stooge(){}
        virtual void slap_stick() = 0;
};

class Larry: public Stooge
{
    public:
        void slap_stick()
        {
            cout << "Larry: poke eyes\n";
        }
};

class Moe: public Stooge
{
    public:
        void slap_stick()
        {
            cout << "Moe: slap head\n";
        }
};

class Curly: public Stooge
{
    public:
        void slap_stick()
        {
            cout << "Curly: suffer abuse\n";
        }
};

class stoogeFactory
{
    public:
    static std::unique_ptr< Stooge > make_stooge(int choice);
};

std::unique_ptr< Stooge > stoogeFactory :: make_stooge(int choice)
{
    switch (choice)
    {
        case 1:
        return std::unique_ptr< Stooge >(new Larry);
    case 2:
        return std::unique_ptr< Stooge >(new Moe);
    case 3:
        return std::unique_ptr< Stooge >(new Curly);
    default:
        throw 5;
    }
    return std::unique_ptr< Stooge >();
}

int main()
{
    int choice;
    while (true)
    {
        cout << "Larry(1) Moe(2) Curly(3) Exit(0): ";
        cin >> choice;

        if (choice == 0)
            break;

        try
        {
           std::unique_ptr< Stooge > myStooge=stoogeFactory::make_stooge(choice);
           myStooge->slap_stick();
        }
        catch(...)
        {
         cout<<"Invalid choice. Please try again."<<endl;
        }
    }
}

Answer 2

You need to write virtual destructor for Stooge. Without virtual destructor, you get undefined behavior if you delete an object of a derived type through a pointer to the base.

class Stooge
{
public:
   virtual void slap_stick() = 0;
   virtual ~Stooge() {}
};

Also in main function, just use smart pointer to from stoogeFactory::make_stooge, no naked new / delete

Answer 3

I see one error at the moment: Stooge must have a virtual destructor. In your case, you delete an object via the base class poitner that is of derived class. Since the base class has no virtual destructor, the deletion results in undefined behavior.

Suggestions:

• Objects should be destoryed the same way the get created, meaning since you let your factory create the objects, let the factory as well destroy them. That way you have no need to know how the factory creates them. new is only one way to do so, and delete only matches new - if the factory was to create objects some other way, you had to destroy them that other way, too, so at the moment your factory is a leaking abstraction.
• Since it's ggod style these days to not use plain pointers but smart pointers, I suggest to give shared_ptr back fromt he factory method. The good thing about that: the destruction gets called automatically, and the deleter is passed during creation, so your factory can insert a deleter consistent to whichever creation method it chooses to use.