1
\$\begingroup\$

Please review my implementation of the factory pattern; it gives a trivial example. Feel free to point out anything I've missed or any code styling tips.

#include <iostream>
#include <string>
#include <memory>
// Abstract base class for a chatbot.
class Chatbot
{
public: 
    virtual std::string GiveChatResponse() const = 0;
};
// Concrete implementation for a customer service chatbot.
class CustomerServiceChatbot : public Chatbot
{
    std::string GiveChatResponse() const
    {
        return std::string{ "Hello, I am a customer service chatbot" };
    };
};
// Concrete implementation for a feedback service chatbot.
class FeedbackServiceChatbot : public Chatbot
{
    std::string GiveChatResponse() const
    {
        return std::string{ "Hello, I am a feedback service chatbot" };
    };
};
// Factory for creating chatbots
class ChatbotFactory
{
public:
    std::shared_ptr<Chatbot> CreateChatbot(const std::string& ChatbotType)
    {
        if (ChatbotType == "CustomerService")
        {
            return std::make_shared<CustomerServiceChatbot>();
        }
        else if (ChatbotType == "FeedbackService")
        {
            return std::make_shared<FeedbackServiceChatbot>();
        }
    };
};
int main()
{
    std::cout << "1 to talk to Customer service, 2 to give feedback, other to close. \n";
    std::string input;
    std::cin >> input;

    ChatbotFactory factory{};
    std::shared_ptr<Chatbot> chatbot = nullptr;
    if (input == "1")
    {
        chatbot = factory.CreateChatbot("CustomerService");
    }
    else if (input == "2")
    {
        chatbot = factory.CreateChatbot("FeedbackService");
    }
    
    if (chatbot)
        std::cout << chatbot->GiveChatResponse();

    return 0;
}
\$\endgroup\$
1
  • \$\begingroup\$ In most use cases, the factory implementation you want is std::make_unique. It creates a smart pointer (which you can cast to a shared or atomic pointer if you need one) to an object constructed from any of the class’ overloaded constructors. \$\endgroup\$
    – Davislor
    Apr 2 at 23:05

1 Answer 1

5
\$\begingroup\$

You have multiple sources of undefined behaviour, I’m afraid. Before we get to that, though, we should talk about the pattern.

Is the method part necessary?

The “factory pattern” is often called the “factory method pattern”, because that is how it is traditionally implemented: using a method. That’s how you’ve done it, and that’s how you’d have to do it in languages like Java.

But that is not the best way to do it in C++. C++ supports object-oriented programming… but it doesn’t require it. And, in fact, in modern C++, object-oriented programming has fallen out of favour.

Consider the dance you have to go through to create a chatbot:

// First you need to create a factory:
auto factory = ChatbotFactory{};

// Then you need to actually use it:
auto chatbot = factory.CreateChatbot("CustomerService");

Is that really necessary? Why couldn’t you just do:

auto chatbot = create_chatbot("CustomerService");

After all, what is the point of actually creating a factory object? There is no use for it. The only thing you’re going to do with it is call the factory method. So why not skip the middleman and just call a factory function? It’s simpler, and more efficient.

If there were some reason why the factory is difficult or expensive to construct, then, sure, it would make sense to only set it up the one time and keep reusing the object via method calls. But without that justification, requiring an object is just clunky, and not idiomatic C++. And as it increases complexity, it increases the need for more and better documentation: for example, is it necessary to keep the factory around for the lifetime of the objects it creates? Better RTFM!

By the laws of KISS and YAGNI, unless you have a good reason to make a factory class with a factory method, just make a factory function.

UB issue 1: the factory method is broken

I can tell at a glance you didn’t compile with warnings turned on… or you just flat-out ignored the warnings. Both of which are big no-nos.

Just for kicks I tried compiling the code in GCC and Clang without even using any warnings options. GCC gave me:

<source>: In member function 'std::shared_ptr<Chatbot> ChatbotFactory::CreateChatbot(const std::string&)':
<source>:44:5: warning: control reaches end of non-void function [-Wreturn-type]
   44 |     };
      |     ^

Clang was perhaps more clear, and without extra clutter:

<source>:44:5: warning: non-void function does not return a value in all control paths [-Wreturn-type]
   44 |     };
      |     ^

Both compilers point to the same place: the end of ChatbotFactory::CreateChatbot():

    std::shared_ptr<Chatbot> CreateChatbot(const std::string& ChatbotType)
    {
        if (ChatbotType == "CustomerService")
        {
            return std::make_shared<CustomerServiceChatbot>();
        }
        else if (ChatbotType == "FeedbackService")
        {
            return std::make_shared<FeedbackServiceChatbot>();
        }
    }; // <-- both compilers point here

See the problem? If not, then consider this: What would happen if someone called factory.CreateChatbot("foo")?

You need to decide what to do if someone requests a non-existent chatbot. Or, more generally, you need to decide how the factory should handle bad requests, and other failures. Your options are:

  1. Throw an exception. This is the best option.
  2. Terminate (such as with an assert). This is also not a bad option, though perhaps a little extreme.
  3. Return an error. This would require changing the return type to something like std::expected<std::shared_ptr<Chatbot>, some_error_type>, which vastly increases the complexity. It would be much more efficient than throwing an exception when there is an error, but assuming that requesting non-existent chatbots should never normally happen, it would be less efficient than exceptions, normally. Unless you are dealing with contexts where you cannot throw exceptions, or getting a request for a non-existent chatbot is not an exceptional situation, exceptions are better.
  4. Return an empty smart pointer. Your code already seems to assume this will happen, sorta. This option is… meh. That’s because it is not obvious that an empty pointer is supposed to be a possible result, and getting one doesn’t tell you why things went wrong.
  5. Return a default chatbot. This is the worst option. There is a special place in hell for coders who write functions that silently swallow errors. But if you are in a situation where this function can never, ever, ever fail… then this is your only option. (Although, in practice, there are already a bunch of ways the function can fail to create a chatbot, so you would have to completely rethink everything if you really want a no-fail chatbot factory.)

Whatever you decide, the real moral here is:

  1. Always compile with all warnings turned on (at least -Wall -Wextra, I’d say).
  2. Never accept code that generates any warnings you have not carefully confirmed are specious.

UB issue #2: the object hierarchy is broken

You are using shared pointers to hold your constructed objects, which is another issue I’ll get to later. But because you are using shared pointers, you are not seeing a serious problem with your object hierarchy.

Shared pointers are incredibly powerful… but also incredibly complex. In fact, few people realize just how powerful and complex they are. I’d wager that even most experienced C++ developers have no clue about shared pointer’s aliasing ability. And I’d wager even fewer know about the quirk that you’ve tripped into.

Let’s take a step back and start simply. Let’s start with a base class and a derived class, with a simple print function to identify themselves. Let’s also give both classes constructors and destructors that print a message:

class base
{
public:
    base() { std::cout << "base constructor\n"; }
    ~base() { std::cout << "base destructor\n"; }

    virtual auto print() -> void { std::cout << "base print\n"; }
};

class derived : public base
{
public:
    derived() { std::cout << "  derived constructor\n"; }
    ~derived() { std::cout << "  derived destructor\n"; }

    auto print() -> void override { std::cout << "    derived print\n"; }
};

Now let’s make a simple program. We won’t use shared_ptr yet… it’s too complex. We’ll start with unique_ptr:

auto main() -> int
{
    std::unique_ptr<derived> p = std::make_unique<derived>();
    p->print();
}

Normally I would just use auto, but I have been very explicit about the type of p here for a reason that will be clear shortly.

You can probably guess the output of this program:

base constructor
  derived constructor
    derived print
  derived destructor
base destructor

Now, the way your factory method works is that it constructs a smart pointer with the derived type, but then converts it (when returning) to a smart pointer of the base type.

To simulate that, we’ll change that one line in main(), and get:

auto main() -> int
{
    std::unique_ptr<base> p = std::make_unique<derived>();
    p->print();
}

If you run this program, you might get:

base constructor
  derived constructor
    derived print
base destructor

(I stress that this is what you might get, because we are triggering undefined behaviour, and whenever you trigger undefined behaviour, you could get anything.)

Notice anything missing? The derived constructor is called… but the derived destructor is not.

What went wrong?

Well, as a general rule, whenever you are creating a base class that is meant to be used polymorphically, you require a virtual destructor.

We could fix that, but before we do, I want to show something interesting. Let’s replace the unique pointers with shared pointers:

auto main() -> int
{
    std::shared_ptr<base> p = std::make_shared<derived>();
    p->print();
}

Try this code, and you’ll get:

base constructor
  derived constructor
    derived print
  derived destructor
base destructor

What the frizzle? It works now! So, is this program now correct? Did shared_ptr fix it?

No. shared_ptr only hid the problem.

Here’s what’s happening. When you do:

std::shared_ptr<base> p = std::make_shared<derived>();
// or (basically the same thing, just less efficient):
std::shared_ptr<base> p = std::shared_ptr<base>{new derived};

You are using constructor number 3, which is:

template <typename U>
// requires std::convertible_to<U*, T*>
explicit shared_ptr(U* p);

The requirement of that constructor is that the pointer will be deleted as if with delete p.

Here’s where the magic happens.

Note that even though p is required to be convertible to a T*, [the standard describes doing delete pnot delete static_cast<T*>(p)]. So when you construct a shared_ptr<base>{derived_pointer}, it does not make a deleter that does auto x = static_cast<base*>(derived_pointer); delete x;. That would trigger UB, because you would be deleting the derived object through a base class pointer (and without a virtual destructor). Instead, it makes a deleter that does delete derived_pointer;, which is deleting the derived object through a derived pointer… which is perfectly fine even without a virtual destructor.

In other words, shared_ptr is smart enough to detect that the type of the pointer you give it (derived) is not the same as the shared pointer’s T type (base), and corrects the deleter for you.

It isn’t doing that to cover up bad code. It is doing it for efficiency. When you give it a derived*, it knows that it needs to delete a derived. So it doesn’t need to do a virtual destructor call. So… it doesn’t. It devirtualizes the destruction.

Which is fine for efficiency… but it just so happens that it also spackles over your coding error. This is a very, very bad thing.

So let’s go back to our unique pointer version that triggered UB. How can we fix it? Simple: any type that will be used polymorphically should have a virtual destructor.

class base
{
public:
    base() { std::cout << "base constructor\n"; }
    virtual ~base() { std::cout << "base destructor\n"; }

    virtual auto print() -> void { std::cout << "base print\n"; }
};

Now the code will always work correctly, whether you are using shared_ptr or not.

In your specific case, the fix would be:

class Chatbot
{
public: 
    virtual std::string GiveChatResponse() const = 0;

    virtual ~Chatbot() = default;
};

Don’t use shared_ptr

After hearing that your code only avoids UB because you used shared_ptr, you must think I’m out of my mind for suggesting you shouldn’t use it.

But in fact, your code is objectively broken, and the fact that shared_ptr hid the problem shows why it is so dangerous. When your code is broken, you want to know.

That’s not the only issue with shared_ptr. It’s also needlessly complex and inefficient. But that’s still not the real reason you shouldn’t use it.

The real reason you should almost never use shared_ptr is right in the name. shared_ptr is meant to model shared ownership. And shared ownership is almost never what you want. Sharing ownership introduces incredible complexity, and opens the door to numerous problems (and I’m not even referring to the problem I explained above; I’m referring to the possibility of cyclical dependencies leading to resources not getting released).

The bottom line is this: Your factory is returning a chatbot. How many owners does that chatbot have? Not how many users; how many owners. In practice, the vast majority of things have a single owner. A single owner means a single point of responsibility. It just makes everything simpler.

It makes no sense to assume by default that a chatbot will be co-owned by multiple objects. In just about every imaginable scenario, it will have a single owner. A… unique owner, if you will.

Which is why unique_ptr is the right smart pointer to use.

You can very simply replace every shared_ptr with unique_ptr and every make_shared with a make_unique, and your program will continue to work the same.

… except for one minor thing: Clang will now detect that you are triggering UB because the lack of a virtual destructor.

In other words, using unique_ptr rather than shared_ptr not only made your program simpler and more efficient… it even detected a bug that was previously hidden.

“But Indi,” you say, “maybe there might be a case where I do want shared ownership of a chatbot?” Okay, I reply, in that very rare, very peculiar situation, this…:

// Given:
auto create_chatbot(std::string const&) -> std::unique_ptr<Chatbot>;

// You can do:
std::shared_ptr<Chatbot> chatbot = create_chatbot("...");
// or:
auto chatbot = std::shared_ptr{create_chatbot("...")};

… works fine. Sure, you lose the optimization you get make_shared(), but given the rarity of the situation (and how little you gain from make_shared()) it’s just not worth it.

(And if you really, really, really, really need that optimization, then it would make more sense to have two version of the factory function—one that makes unique pointers, and one that makes optimized shared pointers—than it does to have one that produces expensive shared pointers when they will almost never be used.)

Miscellaneous minor stuff

Core guideline C.128: Virtual functions should specify exactly one of virtual, override, or final

Your base class is fine (except for the lack of a virtual destructor):

class Chatbot
{
public: 
    virtual std::string GiveChatResponse() const = 0;

    virtual ~Chatbot() = default;
};

But your derived classes…:

class CustomerServiceChatbot : public Chatbot
{
    std::string GiveChatResponse() const
    {
        return std::string{ "Hello, I am a customer service chatbot" };
    };
};

As the guideline states, virtual functions need exactly one of virtual, override, or final. For the derived classes, override is almost always the right choice. So:

class CustomerServiceChatbot : public Chatbot
{
    std::string GiveChatResponse() const override
    {
        return std::string{ "Hello, I am a customer service chatbot" };
    };
};

Speaking of….

The extra string{} is unnecessary

Raw C strings already implicitly convert to strings, so:

class CustomerServiceChatbot : public Chatbot
{
    std::string GiveChatResponse() const override
    {
        return "Hello, I am a customer service chatbot";
    };
};

Repeating the string isn’t wrong, and there are argument for keeping it. Not particularly good ones, but then there aren’t a lot of good ones for dropping it either. It’s up to you, but it’s certainly a lot simpler and less verbose.

And on the topic of strings….

You don’t always need strings

Consider your factory function:

auto CreateChatbot(const std::string& ChatbotType) -> std::unique_ptr<Chatbot>
{
    if (ChatbotType == "CustomerService")
    {
        return std::make_unique<CustomerServiceChatbot>();
    }
    else if (ChatbotType == "FeedbackService")
    {
        return std::make_unique<FeedbackServiceChatbot>();
    }

    throw std::runtime_error{"unknown chatbot type"};
};

So you take a string argument… but the only thing you ever do with it is compare it.

If all you’re going to do is view a string, then you don’t need a string. You can use a string view:

auto CreateChatbot(std::string_view ChatbotType) -> std::unique_ptr<Chatbot>
{
    if (ChatbotType == "CustomerService")
    {
        return std::make_unique<CustomerServiceChatbot>();
    }
    else if (ChatbotType == "FeedbackService")
    {
        return std::make_unique<FeedbackServiceChatbot>();
    }

    throw std::runtime_error{"unknown chatbot type"};
};

Summary

I’m not going to bother to nit on main(), because you’re really more concerned about the factory pattern.

So, in summary:

  • Always compile with warnings on. Never ignore warnings.
  • You don’t need a class for the factory. The factory can be a function.
  • You absolutely need a virtual destructor for an abstract base class that is intended to be used polymorphically.
  • Virtual functions should have exactly one of virtual, override, or final. The base, abstract function should of course be virtual. Derived implementations should have override. Pretty much never use final.
  • Don’t use shared_ptr.
  • Make sure all paths are covered so you don’t “fall off” the end of a function.

(Incidentally, you don’t need a semicolon after defining a class method. Some compiler setups will warn about stray semicolons, and pointless warnings are bad, so you might want to remove them. You do need a semicolon after the class itself, though. And you do need semicolons if you are just declaring class methods, and not also defining them.)

That’s it! Happy coding!

\$\endgroup\$
1
  • \$\begingroup\$ This is really a great answer. \$\endgroup\$ Apr 4 at 1:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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