unique_ptr usage too unwieldy

Question

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 <typename ItemType>
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 <typename DataType>
std::unique_ptr<DataType> EntityDatabase<DataType>::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<DataType>(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<EntityId>(jsonInventory, "Weapon");
    weapon_.reset(static_cast<Weapon*>(ItemDatabase::find(weaponId).release()));

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

    money_ = getValue<Money>(jsonInventory, "Money");

    for (auto& item : jsonInventory.second.get_child("Items"))
    {
        auto itemId = boost::lexical_cast<EntityId, std::string>(item.second.data());
        inventory_.push_back(std::move(ItemDatabase::find(itemId)));
    }
}

How can I improve the code or the design?

Answer 1

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<Item> 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<Item> when you want a std::unique_ptr<Weapon> 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 <typename DataType>
DataType* EntityDatabase<DataType>::find(EntityId const& entityId)

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

 armor_ = static_cast<Armor*>(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<Item> 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 <typename T, typename U>
  void recast(std::unique_ptr<T>& ptr1, std::unique_ptr<U>&& ptr2)
  {
      static_assert(std::is_base_of<U, T>::value, "U is not a base type of T");
      ptr1.reset(static_cast<T*>(ptr2.release()));
  }
  
  ....
  auto armorId  = getValue<EntityId>(jsonInventory, "Armor");
  recast(armor_, ItemDatabase::find(armorId));
  

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