CODEWITHSUNDEEP
c++c++11factorysmart-pointerslifetime
Josh Johnson
Josh Johnson

Reputation: 9422

What type of object should this function return?

Consider this class:

class Widget
{
    Widget::Widget();
    bool initialize();
}

A Widget has the following characteristics:

  1. initialize() must be invoked to fully construct
  2. initialize() may fail
  3. initialize() is expensive

Given that, I am encapsulating creation in factory function that always returns the same Widget instance:

Widget* widget() {
    static auto w = new Widget;
    static auto initialized = false;

    if (!initialized) {
        if (!w->initialize()) {
            return nullptr;
        }
        initialized = true;
    }

    return w;
}

What should the return type of widget() be?

In particular, I'd like to somehow make it clear that the lifetime of the returned Widget will outlast any caller, but without referencing the internal implementation.

  1. Return a raw pointer and add a comment that states "The returned pointer points to an object with static storage duration that will not be deleted before the end of the program". This is simple, but not self-documenting.
  2. Return a std::shared_ptr<Widget>. This is self-documenting, but I don't like that it will introduce completely unnecessary reference counting overhead.
  3. Return a std::unique_ptr<Widget> with a custom deleter function that is a no-op. I think this has the same perceived problem as #2 if the caller converts it into a shared_ptr.

Upvotes: 8

Views: 297

Answers (5)

davidaf
davidaf

Reputation: 351

As other people noted if the factory will only produce one item, factory perhaps is not the right term. It seems a Singleton.

Taking in account that:

  • We will create only one instance of Widget
  • That instance will be constructed the first time someone ask for it (if any)
  • That instance will live until program end AND should be destroyed then
  • Nobody should delete the instance

I'll try something like this:

class Widget {
public:
    static Widget& Instance() {
        static Widget w{};
        return w;
    }

private:
    Widget() {
        // Expensive construction
    }
    Widget(const Widget&) = delete; // avoid copy

};

Upvotes: 1

Chris Drew
Chris Drew

Reputation: 15334

To make lifetime and ownership clearer I would use the conventions of the Singleton pattern and make your function a static getInstance function on the Widget class.

class Widget {
  bool initialize();
 public:
  static Widget* getInstance() {
    static Widget w;
    static bool initialized = false;

    if (!initialized) {
      if (!w.initialize()) {
         return nullptr;
      }
      initialized = true;
    }
    return &w;
  }
};

I think a raw pointer return type documents the fact that the caller is not expected to take ownership and it could be null.

Upvotes: 0

Johann Gerell
Johann Gerell

Reputation: 25581

Herb Sutter's recommendation in this case (item 4 at http://herbsutter.com/2013/05/30/gotw-90-solution-factories/) is to return optional.

There could be one additional reason the function might have returned a pointer, namely to return nullptr to indicate failure to produce an object. Normally it’s better throw an exception to report an error if we fail to load the widget. However, if not being able to load the widget is normal operation and should not be considered an error, return an optional, and probably make the factory noexcept if no other kinds of errors need to be reported than are communicated well by returning an empty optional.

Upvotes: 1

Andre Kostur
Andre Kostur

Reputation: 780

Isn't a raw pointer the right thing to do here? It expresses the restrictions already. It can fail (by returning nullptr), and since it makes no promises about the pointer, callers can't safely cause it to be deleted. You're getting a raw pointer, you can't make the assumption that you're allowed to make any statements about the lifetime of the pointed-to object.

Upvotes: 7

Barry
Barry

Reputation: 303216

I vote for:

boost::optional<Widget&> widget() {
    static Widget w; // no reason for this to be a pointer
    static bool initialized = false;

    if (!initialized) {
        if (!w.initialize()) {
            return boost::none;
        }
        initialized = true;
    }

    return w;
}

It makes it clear that the caller doesn't own the Widget in any way, there's no worry of the caller delete-ing the Widget, and it's clear whether or not the call succeeded.

Upvotes: 11

Related Questions