Should C++ abstract factory provide destroy method for constructed objects?

Question

Consider the following interface (dumb pointers are used because we are still in C++98)

class WidgetMaker {
    virtual Widget* makeWidget() = 0;
};

With the following likely implementation

class SpecificWidgetMaker: public WidgetMaker {
    Widget* makeWidget() {
        return new SpecificWidget();
    }
};

Widget is some base class with virtual destructor, SpecificWidget extends it. My colleagues claim that the WidgetMaker interface should contain the following method

virtual void freeWidget(Widget* widget);

Rationale being is that this way we do not force makeWidget implementations to use standard new allocation, they can use custom pool allocator or always return the same global instance in case widget is stateless or whatever.

I feel such design is generally a bad idea - it complicates client code, violates KISS and YAGNI, makes transition (unlikely in our organization in next 20 years) to unique_ptr harder. Should I trust my feelings? What are the cases when free method as part of abstract factory interface is justified?

Answer 1

A problem with your friend's proposed solution (actually, also with your original one), is that it has an unnecessarily nontrivial protocol. Once you obtain a Widget via makeWidget, you need to remember to deallocate it (either directly, or by calling some method of the factory). This is known to be fragile - it will either break down quickly (causing Widget leaks), or really complicate the client code.

If you look at the interface of std::shared_ptr::shared_ptr(...), you can see that it can take a custom deleter object.

Thus, perhaps you could typedef (or the equivalent) what exactly is a Widget smart pointer:

using WidgetPtr = std::shared_ptr<Widget, ...>

In case you later decide that the factory needs to perform some action when a Widget is deallocated, you can change the typedef to one which uses a custom deleter, and this custom deleter can notify the factory that an object is being deleted.

The main advantage of this is that it removes the onus of remembering to deallocate a Widget from the user.

Answer 2

I would say that there is no general rule of thumb to follow here. It all depends on the details of your Widgets implementation.

If everything that needs to be done to properly destroy an instance of any Widget subclass can be handled in its ordinary destructor, declaring an explicit freeWidget() in your factor serves no useful purpose. It does not add any value.

If, on the other hand, something needs to be done that cannot be handled, for whatever reason, in the destructor, then, obviously, you need an explicit methods to destroy your widgets.

Perhaps there isn't a need for any special handling, of this sort, at this time, but you foresee the need to it in the future. In that case, it makes sense to declare an explicit freeWidget() method, to avoid rewriting a bunch of code later.

If you decide to go with the freeWidget() approach, one thing you might consider doing is making the destructors of all subclasses private (most likely with some suitable friend declaration so something can actually destroy these things), to enforce this policy.

One example why you might want to have an explicit freeWidget() would be exceptions. Throwing exceptions from destructors is ...touchy. It's allowed, but it comes with certain ...restrictions. So, if there's a possibility that destroying your widget might throw an exception, using a freeWidget() will allow you to have more control over throwing an exception, in that instance, and correctly cleaning up the destroyed widget before doing so.