Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

share|improve this question

migrated from stackoverflow.com Feb 4 '13 at 2:55

This question came from our site for professional and enthusiast programmers.

    
Could be stoogeFactory::make_stooge(int severity). –  Mark Garcia Jan 30 '13 at 7:21
1  
Yeah, pretty much. –  SauceMaster Jan 30 '13 at 7:21
add comment

3 Answers 3

  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;
        }
    }
}
share|improve this answer
    
With unique_ptr you have to state the correct deleter type and thus are leaking the type of creation of the objects (operator new in this case). See my answer. –  Arne Mertz Jan 30 '13 at 7:31
    
@ArneMertz shared_ptr is ok, but in this specific case, unique_ptr is better. You do not need a custom deleted, and I am not sure what you mean with leaking the type –  BЈовић Jan 30 '13 at 7:54
    
It's leaking the type of creation - so to say the manner in which the objects are created. I'm sorry if that's not understandable, English is not my mother tongue. You are right that in this case there is no custom deleter, but that means std::default_delete is encoded in the unique_ptr's type. That means it says explicitly that the factory allocates memory using new. But since Factory Method is all about abstracting away the creation of the object, it should as well abstract away the means of memory accuisition. So why is unique_ptr "better"? –  Arne Mertz Jan 30 '13 at 8:10
    
Alternatively, you could add a fourth stooge, NullStooge, which does nothing, and return that when the stooge is unknown. –  Peter Wood Jan 30 '13 at 8:10
1  
@PeterWood I would recommend against that. If you give an invalid argument to the factory, it should issue an error. Returning a "not-a-real-object" object is imo even worse than returning NULL, since everything seems to work well, but not the way you expect. –  Arne Mertz Jan 30 '13 at 8:14
show 9 more comments

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

share|improve this answer
add comment

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.
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.