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();

        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));
        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");

    auto armorId  = getValue<EntityId>(jsonInventory, "Armor");

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

    for (auto& item : jsonInventory.second.get_child("Items"))
        auto itemId = boost::lexical_cast<EntityId, std::string>(item.second.data());

How can I improve the code or the design?


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
    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");
    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.

  • \$\begingroup\$ Thank you for your input! You are correct with almost all assumptions, except that EntityDatabase and, by extension, ItemDatabase are static. My first approach was exactly to declare armor_ as Armor*. I did this because I wanted to constrain the types of items that could act as armor or weapons, even though they are items. I'm inclined to agree that this is an abuse of polymorphism, but aside from declaring Weapon and Armor not a derivative of Item, I didn't and don't know how to otherwise solve it. \$\endgroup\$ – IAE Apr 1 '13 at 9:31
  • \$\begingroup\$ I added some more information based on the assumptions you made and further snippets I thought to be illuminating. \$\endgroup\$ – IAE Apr 1 '13 at 9:44
  • \$\begingroup\$ I think in this case there isn't all that much you can do. Unfortunately, C++ syntax is not concise at the best of times. The code is ugly, true, but it's not too hard to understand what is going on. \$\endgroup\$ – Yuushi Apr 3 '13 at 1:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.