Return a unique_ptr by reference

Question

So I've solved this problem, but I need your opinion if what I did is best practice.

A simple class holds a vector of unique_ptrs to order objects. I will explain the member variable null_unique below.

class order_collection {
    typedef std::unique_ptr<order> ord_ptr;
    typedef std::vector<ord_ptr> ord_ptr_vec;
    ord_ptr_vec orders;
    ord_ptr null_unique;
public:
    ...
    const ord_ptr & find_order(std::string);
    ....

So I need the users of this class to get access to the order unique_ptr if found. However I'm not going to move the object out of the vector so I'm returning the unique_ptr as const ref. My implementation of the find_order method:

const order_collection::ord_ptr & order_collection::find_order(std::string id) {
    auto it = std::find_if(orders.begin(),orders.end(),
                           [&](const order_collection::ord_ptr & sptr) {
                               return sptr->getId() == id;
                           });

    if (it == orders.end())
        return null_unique; // can't return nullptr here
    return *it;
}

Since I'm returning by reference I can't return a nullptr. If I try to do so, I get warning : returning reference to a temporary. And if nothing is found the program crashes. So I added a unique_ptr<order> member variable called null_unique and I return it when find doesn't find an order. This solves the problem and warning is gone and doesn't crash when no order is found.

However I'm doubting my solution as it make my class ugly. Is this the best practice for handling this situation?

Answer 1

You should only return and accept smart pointers when you care about their ownership semantics. If you only care about what they're pointing to, you should instead return a reference or a raw pointer.

Since you're returning a dummy null_unique, it is clear that the caller of the method doesn't care about the ownership semantics. You can also have a null state: you should therefore return a raw pointer:

order* order_collection::find_order(std::string id) {
    auto it = std::find_if(orders.begin(),orders.end(),
                           [&](const order_collection::ord_ptr & sptr) {
                               return sptr->getId() == id;
                           });

    if (it == orders.end())
        return nullptr;
    return it->get();
}

Answer 2

It doesn't really make sense to return a unique_ptr here, reference or otherwise. A unique_ptr implies ownership over the object, and those aren't really the semantics being conveyed by this code.

As suggested in the comments, simply returning a raw pointer is fine here, provided that your Project Design explicitly prohibits you or anyone on your team from calling delete or delete[] outside the context of the destructor of a Resource-owning object.

Alternatively, if you either have access to Boost or C++17, a std::optional<std::reference_wrapper<order>> might be the ideal solution.

std::optional<std::reference_wrapper<order>> order_collection::find_order(std::string id) {
    auto it = std::find_if(orders.begin(),orders.end(),
                       [&](const order_collection::ord_ptr & sptr) {
                           return sptr->getId() == id;
                       });

    if (it == orders.end())
        return {}; //empty optional object
    return **it; //will implicitly convert to the correct object type.
}

/*...*/

void func() {
    auto opt = collection.find_order("blah blah blah");
    if(!opt) return;
    order & ord = opt->get();
    /*Do whatever*/
}

(EDIT: In testing on the most recent version of MSVC 2017, it looks like std::reference_wrapper<T> will happily do an implicit conversion to T& if you tell it to. So replacing opt->get() with *opt should work exactly the same.)

As long as I'm here, I might point out that a std::vector<std::unique_ptr<type>> object has a very "Code Smell" sense to it. std::vector<type> implies ownership of the object as is, so unless you have a good reason to prefer this (maybe the objects are large, unmovable/uncopyable, and you need to insert and remove entries frequently? Maybe this is a polymorphic type?), you're probably better off reducing this to a simple std::vector.

EDIT:

The boost version is subtly different, because boost::optional has no restrictions against "optional references", which are specifically forbidden by the C++ Standard Library's version of std::optional. The boost version is actually going to be slightly simpler:

//return type changes, nothing else changes
boost::optional<order&> order_collection::find_order(std::string id) {
    auto it = std::find_if(orders.begin(),orders.end(),
                       [&](const order_collection::ord_ptr & sptr) {
                           return sptr->getId() == id;
                       });

    if (it == orders.end())
        return {}; //empty optional object
    return **it; //will implicitly convert to the correct object type.
}

/*...*/

//Instead of calling opt->get(), we use *opt instead.
void func() {
    auto opt = collection.find_order("blah blah blah");
    if(!opt) return;
    order & ord = *opt;
    /*Do whatever*/
}