HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Is this a correct Factory Method implementation?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thismethodfactorycorrectimplementation

Problem

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 
#include 
using namespace std;

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

class Win7Widget : public Widget {
public:
    void draw() { cout  win7fact(new Win7Factory);
    unique_ptr win8fact(new Win8Factory);

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

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

    return 0;
}

Solution

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 Create() = 0;


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

using WidgetPtr = std::unique_ptr;


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.

Code Snippets

virtual std::unique_ptr<Widget> Create() = 0;
using WidgetPtr = std::unique_ptr<Widget>;
virtual ~Factory() = default;

Context

StackExchange Code Review Q#64740, answer score: 4

Revisions (0)

No revisions yet.