Implementing Sample Abstract Factory Pattern using Smart Pointers in C++

Question

Was trying to implement and example code to smart pointer from link. Is it a good practice? To go with all unique pointers or should we use shared pointers in this case? How can i improve this ?

#include <iostream>
#include <string>
#include<memory>

void LOG(std::string strText)
{
    std::cout << strText << std::endl;
}

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

class WindowsBtn : public Widget
{
public:
    void draw() {
        LOG("Windows Button");
    }
};

class WindowsMenu : public Widget
{
public:
    void draw() {
        LOG("Windows Menu");
    }
};

class LinuxBtn : public Widget
{
public:
    void draw() {
        LOG("Linux Button");
    }
};

class LinuxMenu : public Widget
{
public:
    void draw() {
        LOG("Linux Menu");
    }
};
/*
class WidgetFactory
{
public:
    virtual Widget* CreateButton() = 0;
    virtual Widget* CreateMenu() = 0;
};

class WindowsFactory : public WidgetFactory{
public:
    Widget* CreateButton() {
        return (new WindowsBtn);
    }

    Widget* CreateMenu() {
        return (new WindowsMenu);
    }

};

class LinuxFactory : public WidgetFactory{
public:
    Widget* CreateButton() {
        return (new LinuxBtn);
    }

    Widget* CreateMenu() {
        return (new LinuxMenu);
    }

};

class Client
{
    WidgetFactory * factory;
public:
    Client(WidgetFactory* factory) :factory(factory)
    {}

    void draw(){
        Widget * wd = factory->CreateButton();
        wd->draw();
        display1();
        display2();
    }
    void display1()
    {
        Widget * w[] = {
            factory->CreateButton(),
            factory->CreateMenu()
        };
        w[0]->draw();
        w[1]->draw();
    }
    void display2()
    {
        Widget * w[] = {
            factory->CreateMenu(),
            factory->CreateButton()
        };
        w[0]->draw();
        w[1]->draw();
    }

};
*/
class SmartWidgetFactory
{
public:
    virtual std::unique_ptr<Widget> CreateButton() = 0;
    virtual std::unique_ptr<Widget> CreateMenu() = 0;
};

class SmartWindowsFactory : public SmartWidgetFactory {
public:
    std::unique_ptr<Widget> CreateButton() {
        return std::make_unique<WindowsBtn>();
    }

    std::unique_ptr<Widget> CreateMenu() {
        return  std::make_unique<WindowsMenu>();
    }
};

class SmartLinuxFactory : public SmartWidgetFactory {
public:
    std::unique_ptr<Widget> CreateButton() {
        return  std::make_unique<LinuxBtn>();
    }

    std::unique_ptr<Widget> CreateMenu() {
        return std::make_unique<LinuxMenu>();
    }
};

class SmartClient
{
    std::unique_ptr<SmartWidgetFactory> factory;
public:
    SmartClient(std::unique_ptr<SmartWidgetFactory> oFactory)
    {
        factory = std::move(oFactory);
    }

    void draw() {
        std::unique_ptr<Widget> wd = factory->CreateButton();
        wd->draw();
        display1();
        display2();
    }
    void display1()
    {
        std::unique_ptr<Widget> w[] = {
            factory->CreateButton(),
            factory->CreateMenu()
        };
        w[0]->draw();
        w[1]->draw();
    }
    void display2()
    {
        std::unique_ptr<Widget> w[] = {
            factory->CreateMenu(),
            factory->CreateButton()
        };
        w[0]->draw();
        w[1]->draw();
    }
};

int _tmain(int argc, _TCHAR* argv[])
{

    /*WidgetFactory *factory;
    factory = new LinuxFactory;
    Client *c = new Client(factory);
    c->draw();*/

    std::unique_ptr< SmartWidgetFactory > oFactory = std::make_unique<SmartLinuxFactory>();
    std::unique_ptr<SmartClient> oClient = std::make_unique<SmartClient>( std::move(oFactory) );
    oClient->draw();

    return 0;
}

Answer 1

First, the big bug. Your Widget class has no virtual destructor! This means that every time you return a unique_ptr<Widget>, you're losing the information about what kind of widget needs destroying.

Clang will actually diagnose this mistake for you. No matter what compiler you use, make sure you turn on warnings via -Wall -Wextra (or -W4 on MSVC).

---

int _tmain(int argc, _TCHAR* argv[])

This is a non-standard main routine. Maybe I'm a POSIX chauvinist, but at least for posting publicly, I'd think it would be more appropriate to use

int main(int, char **)

or even better,

int main()

(since you don't use either of those parameters for anything).

Also notice that I'm using char** instead of char*[], because in C and C++, a function parameter cannot be an array. It is pointless to deceive yourself and mean to deceive others.

---

SmartClient(std::unique_ptr<SmartWidgetFactory> oFactory)

This constructor should be marked explicit. In fact, every constructor you ever write (except for copy constructors and move constructors) should be explicit. If you leave off the explicit, then you're enabling people to write code like

std::unique_ptr<SmartWidgetFactory> pf = ...;
SmartClient sc = std::move(pf);

or even

SmartClient sc = nullptr;

and it'll compile, and they won't realize what they did wrong until runtime. It's better to catch this kind of thing at compile time. Mark all your constructors explicit.

---

std::unique_ptr<Widget> CreateButton()

If you intended this function to override the virtual function in the base class, you should have marked it override. (It may still override the virtual function in the base class, even without override, if you spelled everything correctly; but don't bet your workday on it. Mark all overriding functions override and catch your bugs at compile time.)

Also, consider whether it makes conceptual sense that "creating a button" is a non-const method of the factory. Maybe it does; I tend to think it doesn't. So I would write this as

std::unique_ptr<Widget> CreateButton() const override

---

std::unique_ptr<SmartClient> oClient = std::make_unique<SmartClient>( std::move(oFactory) );

Why does this need to be heap-allocated?

auto oClient = SmartClient(std::move(oFactory));

Even if you want to keep using make_unique for some reason, consider replacing the cumbersome type name std::unique_ptr<SmartClient> with auto:

auto oClient = std::make_unique<SmartClient>(std::move(oFactory));

Notice I've also normalized the whitespace while I'm at it.

---

void LOG(std::string strText)

Pass large expensive-to-copy objects, such as std::string, by const reference.

void LOG(const std::string& text)

If you are on C++17, consider replacing const std::string& parameters with std::string_view parameters:

void LOG(std::string_view text)

Be aware that you should use string_view only in places where it would make equal sense to use a reference type (so, parameters, and that's it).

Also be aware that when people see LOG("foo") in your code, they will assume that LOG is a macro, because it's ALL UPPER CASE. If you mean for people to assume it's a function, call it print_to_log or something like that.

Answer 2

To answer your specific question:

To go with all unique pointers or should we use shared pointers in this case?

You're absolutely correct to return std::unique_ptr values. This indicates that ownership of the pointed-to object is transferred to the caller. If you were to return std::shared_ptr, then that would imply that the factory retained a share of the ownership (e.g. by retaining an object pool, or something).

Remember that once the caller has a unique pointer, it is able to convert it to a shared pointer if it needs to (but the opposite conversion is obviously not possible, except via the raw pointer).

Answer 3

I've noticed a few small things:

• You don't need to flush the stream every time you log something. A simple newline will do.

• A more suitable log stream provided by C++ would be std::clog. You may, however, need to flush this stream.

• Your formatting is inconsistent. You have spaces in places where you don't have spaces elsewhere, including but not limited to: in template parameters and after return. This makes the code slightly hard to read.