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?

At least, you should push for moving to smart pointers as soon as possible, it will save a lot of work and avoid unnecessary bugs. — Erik Alapää, Feb 10 '16 at 13:30
Do you have to use C++98? If you do this should be tagged with that. — NathanOliver, Feb 10 '16 at 13:32
TBH I think smart pointers is the correct solution to this problem as they can contain custom deleters. This proposal forces you to keep using *raw pointers* — Galik, Feb 10 '16 at 13:40
If you can't use std::shared_ptr because of C++98, have you evaluated the cost of implementing your own equivalent? Ideally, something you could replace with a typedef or type alias once you have a more modern C++ standard library available. — TBBle, Feb 10 '16 at 14:24

score 7 · Accepted Answer · answered Feb 10 '16 at 13:29

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.

I know. It's a problem with the standard version of programming language we are using. And also people not trusting in boost. The client of WidgetMaker is likely to take exclusive ownership so shared ptr seems like overhead (currently it uses some ad-hoc scoped_ptr). I'll try to propose boost::shared_ptr. — Muxecoid, Feb 10 '16 at 13:42
@Muxecoid Regarding your first point, note that it's part of the standard now (not boost exclusively anymore). — Ami Tavory, Feb 10 '16 at 13:46
@Muxecoid Oh, just got your point about the standard version you're using. Am leaving the previous comment for future ref., though. — Ami Tavory, Feb 10 '16 at 13:51

score 0 · Answer 2 · answered Feb 10 '16 at 13:31

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.

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

2 Answers2