unique_ptr usage too unwieldy

I'm creating a MUD and am running into some design issues regarding Items.

Items are stored in an ItemDatabase after being read from a JSON file.

template 
static void load(std::string const& filename)
{
    PropertyTree propertyTree;
    boost::property_tree::read_json(filename, propertyTree);

    for (auto& jsonItem : propertyTree)
    {
        Item *item = new ItemType();
        item->fromJson(jsonItem);

        database_[item->id()] = item;
    }
}

At first, every Database::find() returned a shallow copy of the item, however, this quickly becomes dangerous and erroneous when items have a finite usage, e.g. a life potion that is consumed after one charge.

Therefore, I want to start returning deep copies as std::unique_ptrs because each Item is unique and is possessed by only one entity. This avoids dangerous dangling pointers, if the Item is not maintained or destroyed by the caller:

template 
std::unique_ptr EntityDatabase::find(EntityId const& entityId)
{
    auto found = std::find_if(std::begin(database_), std::end(database_), 
                              [&entityId](Container::value_type const& candidate)
                              {
                                  return (candidate.second->id() == entityId);
                              });

    if (found != std::end(database_))
        return std::unique_ptr(new DataType(*found->second));
    else
        return nullptr;
}

However, the resulting code is worse than hideous. Consider turning an Item into a Weapon:

```
void Inventory::fromJson(PropertyTree::value_type const& properties)
{
auto jsonInventory = *properties.second.find("Inventory");

auto weaponId = getValue(jsonInventory, "Weapon");
weapon_.reset(static_cast(ItemDatabase::find(weaponId).release()));

auto armorId = getValue(jsonInventory, "Armor");
armor_.reset(static_cast(ItemDatabase::find(armorId).release()));

money_ = getValue(jsonInventory, "Money");

for (auto& item

Let me restate the problem, because it wasn't 100% clear to me from what you've written. I personally think you've left out a bit too much code. I'll state my assumptions of what is going on, but ideally this should be clear from what is posted. Firstly, I assume EntityDatabase is a class, not a namespace. Secondly, I assume ItemDatabase is declared as something like:

EntityDatabase ItemDatabase;

Further, I assume you have some kind of inheritance hierarchy that looks something like:

class Item
{
    Item();
    virtual ~Item();
    //....
}

class Weapon : public Item
{ 
    //...
}

class Armor : public Item
{
    //...
}

The problem then is that ItemDatabase.find(...) (I assume it's meant to be ., not ::) will return a std::unique_ptr when you want a std::unique_ptr or some other derived class of Item.

This seems less a problem of unique_ptr and more the fact that you're kind of abusing polymorphism. Let's assume you didn't return a unique_ptr, just a normal Item*:

template 
DataType* EntityDatabase::find(EntityId const& entityId)

The only thing that would change then is that armor_ would be declared as Armor* instead of std::unique_ptr, so it would become:

armor_ = static_cast(ItemDatabase::find(armorId));

Hence, the only things that really change are the calls to release and reset. This is not significantly less ugly than what is already there. If armour_ were declared as std::unique_ptr then you could of course do the following:

armor_ = std::move(ItemDatabase::find(armorId));

Again, this is all built on a lot of assumptions since there is quite a bit of context missing.

For fixes, I suppose the options are:

-
Leave it as is. It's fairly ugly, but not that much uglier than the code you'd have for raw pointers.

-
Fix your inheritance hierarchy. The fact that you're having to cast things back and forth suggests that perhaps polymorphism isn't the correct solution here. Does each item need to know its specific type? Is there a virtual function you can add to Item that will fix this? I don't know, there's not enough context from the code you've posted.

-
Finally, make a function to limit the uglyness to one spot:

template 
void recast(std::unique_ptr& ptr1, std::unique_ptr&& ptr2)
{
    static_assert(std::is_base_of::value, "U is not a base type of T");
    ptr1.reset(static_cast(ptr2.release()));
}

....
auto armorId  = getValue(jsonInventory, "Armor");
recast(armor_, ItemDatabase::find(armorId));

This is still pretty ugly, but if used often, might be slightly less offensive.

StackExchange Code Review Q#24566, answer score: 4