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.

I'm learning about design pattern and I tried to implement a Factory Method example, based on the GoF book.

Can I say that this is a correct implementation of it ? If not I will be glad to find out what are the pitfalls.

#include <iostream>
#include <vector>
using namespace std;


class Widget {
public:
    virtual void draw() = 0;
};

class Win7Widget : public Widget {
public:
    void draw() { cout << "Win 7 widget" << endl; }
};

class Win8Widget : public Widget {
public:
    void draw() { cout << "Win 8 widget" << endl; }
};


class Factory {
public:
    virtual Widget* Create() = 0;
    virtual ~Factory() {}
};

class Win7Factory: public Factory {
public:
    Widget* Create() { return new Win7Widget; }
};

class Win8Factory: public Factory {
public:
    Widget* Create() { return new Win8Widget; }
};


int main()
{
    unique_ptr<Factory> win7fact(new Win7Factory);
    unique_ptr<Factory> win8fact(new Win8Factory);

    vector<Widget*> widgets;
    widgets.push_back(win7fact->Create());
    widgets.push_back(win8fact->Create());

    for(const auto& f : widgets)
    {
        f->draw();
    }

    return 0;
}
share|improve this question

2 Answers 2

Don't use dynamic allocation if automatic objects will do:

unique_ptr<Factory> win7fact(new Win7Factory);
unique_ptr<Factory> win8fact(new Win8Factory);

Should be:

Win7Factory win7fact;
Win8Factory win8fact;
share|improve this answer

Overall, it is pretty good.

The one thing you should improve is returning a smart pointer from the factory method. The way it is now, returning a raw pointer, it is up to the caller to decide how the memory management of the object is to be done. This is sub-optimal, as it leaves room for error. In your example, the widgets will leak, as nobody is explicitly owning that memory. So Create() should return either a shared or unique pointer. Unique seems more fit for this case:

virtual std::unique_ptr<Widget> Create() = 0;

Or use a type alias to make the code less verbose:

using WidgetPtr = std::unique_ptr<Widget>;

Avoid using namespace std;. It is not that much more typing, and there are gains to it.


The destructor of Factory is empty, so you should make it a default (C++11 feature):

virtual ~Factory() = default;

No need to explicitly return 0 from main(). The compiler adds it automatically for main if omitted.

share|improve this answer

Your Answer

 
discard

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.