C++ Inventory System

Question

I have recently been working a lot on a project for a system that represents an inventory and item usage in a game, with one of the important features is that it should be reusable in multiple projects. I think I managed to do this quite well, keeping the required interface as minimum as possible. I have come to a point where I think the project is finished, but I would like to get feedback on where I can still improve the code. It's really long, so I'm not sure if I should post everything here. I will just post the main Inventory header here, and the rest of the code can be found on github. If I do need to copy all code here, let me know.

Inventory.h

#pragma once

#include "IItem.h"
#include "ItemDispatcher.h"
#include <memory>
#include <functional>
#include <string>
#include <unordered_map>
#include <string_view>
#include <type_traits>
#include <utility>



template<unsigned int MAX_SIZE, typename GameObjTy = temp::GameObject, typename ItemTy = IItem>
class Inventory
{
private:

    class Traits //class to simulate namespace inside class
    {
    public:

        /*HasUseMethod type trait*/

        template<typename _Ty, typename = std::void_t<>>
        struct HasUseMethodHelper : std::false_type 
        {
        };

        template<typename _Ty>
        struct HasUseMethodHelper<_Ty, std::void_t<decltype(std::declval<_Ty>().use(std::declval<ItemDispatcher<GameObjTy>&>()))>> : std::true_type
        {
        };

        template<typename _Ty>
        struct HasUseMethodT : HasUseMethodHelper<_Ty>::type
        {
        };

        template<typename _Ty> using HasUseMethod = typename HasUseMethodT<_Ty>::type;
        template<typename _Ty> static constexpr bool HasUseMethodV = HasUseMethod<_Ty>::value;

        /*HasEquippabmeMethod type trait*/

        template<typename _Ty, typename = std::void_t<>>
        struct HasEquippableMethodHelper : std::false_type
        {
        };

        template<typename _Ty>
        struct HasEquippableMethodHelper<_Ty, 
            std::void_t<decltype(std::is_same_v<decltype(std::declval<_Ty>().equippable()), bool >)>> : std::true_type
        {
        };

        template<typename _Ty>
        struct HasEquippableMethodT : HasEquippableMethodHelper<_Ty>::type
        {
        };

        template<typename _Ty> using HasEquippableMethod = typename HasEquippableMethodT<_Ty>::type;
        template<typename _Ty> static constexpr bool HasEquippableMethodV = HasEquippableMethod<_Ty>::value;

        /*HasIsEquipped type trait*/

        template<typename _Ty, typename = std::void_t<>>
        struct HasIsEquippedMethodHelper : std::false_type
        {
        };

        template<typename _Ty>
        struct HasIsEquippedMethodHelper<_Ty, 
            std::void_t<decltype(std::is_same_v<decltype(std::declval<_Ty>().is_equipped()), bool >)>> : std::true_type
        {
        };

        template<typename _Ty>
        struct HasIsEquippedMethodT : HasIsEquippedMethodHelper<_Ty>::type
        {
        };

        template<typename _Ty> using HasIsEquippedMethod = typename HasIsEquippedMethodT<_Ty>::type;
        template<typename _Ty> static constexpr bool HasIsEquippedMethodV = HasIsEquippedMethod<_Ty>::value;

        /*HasSetEquip type trait*/

        template<typename _Ty, typename = std::void_t<>>
        struct HasSetEquipMethodHelper : std::false_type
        {
        };

        template<typename _Ty>
        struct HasSetEquipMethodHelper<_Ty, 
            std::void_t<decltype(std::is_same_v<decltype(std::declval<_Ty>().set_equip(std::declval<bool>())), void >)>> 
                : std::true_type
        {
        };

        template<typename _Ty>
        struct HasSetEquipMethodT : HasSetEquipMethodHelper<_Ty>::type
        {
        };

        template<typename _Ty> using HasSetEquipMethod = typename HasSetEquipMethodT<_Ty>::type;
        template<typename _Ty> static constexpr bool HasSetEquipMethodV = HasSetEquipMethod<_Ty>::value;

        /*HasUnequipMethod type trait*/

        template<typename _Ty, typename = std::void_t<>>
        struct HasUnequipMethodHelper : std::false_type
        {
        };

        template<typename _Ty>
        struct HasUnequipMethodHelper<_Ty, 
            std::void_t<decltype(std::is_same_v<decltype(std::declval<_Ty>().unequip(std::declval<GameObjTy*>())), void >)>> 
                : std::true_type
        {
        };

        template<typename _Ty>
        struct HasUnequipMethodT : HasUnequipMethodHelper<_Ty>::type
        {
        };

        template<typename _Ty> using HasUnequipMethod = typename HasUnequipMethodT<_Ty>::type;
        template<typename _Ty> static constexpr bool HasUnequipMethodV = HasUnequipMethod<_Ty>::value;

        /*HasReusableMethod type trait*/

        template<typename _Ty, typename = std::void_t<>>
        struct HasReusableMethodHelper : std::false_type
        {
        };

        template<typename _Ty>
        struct HasReusableMethodHelper<_Ty, 
            std::void_t<decltype(std::is_same_v<decltype(std::declval<_Ty>().reusable()), bool >)>> 
                : std::true_type
        {
        };

        template<typename _Ty>
        struct HasReusableMethodT : HasReusableMethodHelper<_Ty>::type
        {
        };

        template<typename _Ty> using HasReusableMethod = typename HasReusableMethodT<_Ty>::type;
        template<typename _Ty> static constexpr bool HasReusableMethodV = HasReusableMethod<_Ty>::value;

        template<typename _Ty, typename = std::void_t<>>
        struct HasStackableMethodHelper : std::false_type
        {
        };

        template<typename _Ty>
        struct HasStackableMethodHelper<_Ty, 
            std::void_t<decltype(std::is_same_v<decltype(std::declval<_Ty>().stackable()), bool>)>>
                : std::true_type
        {
        };

        template<typename _Ty>
        struct HasStackableMethodT : HasStackableMethodHelper<_Ty>::type
        {
        };

        template<typename _Ty> using HasStackableMethod = typename HasStackableMethodT<_Ty>::type;
        template<typename _Ty> static constexpr bool HasStackableMethodV = HasStackableMethod<_Ty>::value;

        template<typename _Ty>
        struct IsValidItemT
        {
            static constexpr bool value =
                HasEquippableMethodV<_Ty>
                && HasUseMethodV<_Ty>
                && HasIsEquippedMethodV<_Ty>
                && HasSetEquipMethodV<_Ty>
                && HasEquippableMethodV<_Ty>
                && HasReusableMethodV<_Ty>
                && HasStackableMethodV<_Ty>;
        };

        template<typename _Ty> using IsValidItem = typename IsValidItemT<_Ty>::type;
        template<typename _Ty> static constexpr bool IsValidItemV = IsValidItemT<_Ty>::value;
    };

public:
    static_assert(Traits::IsValidItemV<ItemTy>, "Item type is invalid. It should provide methods listed in documentation");

    class Exception
    {
    private:
        std::string msg;
    public:
        explicit inline Exception(std::string_view error) : msg {error} {}
        inline std::string_view what() { return msg; }
    };

    using game_object_type = GameObjTy; 
    using item_type = ItemTy; 
    using item_pointer = std::unique_ptr<item_type>;

    using game_object_pointer = game_object_type*;
    using inventory_type = std::unordered_map<std::string, std::pair<item_pointer, unsigned int>>;
    using iterator = typename inventory_type::iterator;
    using const_iterator = typename inventory_type::const_iterator;
    using size_type = typename inventory_type::size_type;

    explicit Inventory(game_object_pointer owner);
    Inventory(Inventory const& other) = delete;
    Inventory(Inventory&& other);

    Inventory& operator=(Inventory const& other) = delete;
    Inventory& operator=(Inventory&& other);

    inventory_type const& contents() const;
    inventory_type& contents();

    /*Adds a new item, stacked on top of an item with the same ID. The id parameter will be used to access the item*/
    template<typename ItemT>
    void addItem(std::string_view id);

    /*constructs a new item and adds it to the inventory. The id parameter will be used to access the item*/
    template<typename ItemT, typename... Args>
    void emplaceItem(std::string_view id, Args... args);

    void useItem(std::string_view name, game_object_pointer target = nullptr);

    /*The iterators invalidate when a new Item is added to the inventory*/
    iterator getItem(std::string_view name);
    /*The iterators invalidate when a new Item is added to the inventory*/
    const_iterator getItem(std::string_view name) const;

    void removeItem(std::string_view name);

    iterator begin();
    iterator end();

    const_iterator cbegin() const;
    const_iterator cend() const;

    size_type max_size() const;
    size_type size() const;

    /*Merges inventory A with this inventory. Leaves other empty, unless this inventory is full, in which case the leftover
      elements will be deleted*/ //#TODO: leftover elements are left in old inventory?
    template<unsigned int N>
    void merge(Inventory<N, GameObjTy, ItemTy>& other);

    template<unsigned int N>
    bool merge_fits(Inventory<N, GameObjTy, ItemTy> const& other);

    /*Transfers item with name parameter into the inventory specified in destination, unless destination does not have enough
     *space left*/
    template<unsigned int N>
    void transfer(Inventory<N, GameObjTy, ItemTy>& destination, std::string_view name);

    bool empty() const;
    bool full() const;

    void setOwner(game_object_pointer owner);
    game_object_pointer getOwner() const;

    void clear();
    unsigned int getItemCount(std::string_view id) const;

private:
    size_type m_size = 0;

    inventory_type m_items { MAX_SIZE };
    game_object_pointer m_owner { nullptr };

    //these functions are private so you cannot accidentally pass an invalid iterator to one of these, causing undefined behavior

    void useItem(iterator pos, game_object_pointer target = nullptr); 
    void removeItem(iterator pos);

    inline Exception InventoryFullException() const { return Exception {"Inventory is full"}; }
    inline Exception InvalidItemTypeException() const { return Exception {"Item type must be derived from Inventory::ItemTy, which defaults to IItem"}; }
    inline Exception InvalidItemException() const { return Exception { "Invalid item name" }; }
    inline Exception InvalidStackException() const { return Exception {"Tried to stack a non-stackable item"}; }
    inline Exception InvalidIDException() const { return Exception {"ID not found in inventory"}; }
};

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
Inventory<MAX_SIZE, GameObjTy, ItemTy>::Inventory(game_object_pointer owner) : m_owner(owner)
{

}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
Inventory<MAX_SIZE, GameObjTy, ItemTy>::Inventory(Inventory&& other) : m_owner(std::move(other.m_owner)), m_items(std::move(other.m_items)), m_size(other.m_size)
{
    other.m_owner = nullptr;
    other.m_items = inventory_type {};
    other.m_size = 0;
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
Inventory<MAX_SIZE, GameObjTy, ItemTy>& Inventory<MAX_SIZE, GameObjTy, ItemTy>::operator=(Inventory<MAX_SIZE, GameObjTy, ItemTy>&& other)
{
    // #WARNING: Self assignment check is missing
    m_owner = other.m_owner;
    m_items = std::move(other.m_items);
    m_size = other.m_size;

    other.m_owner = nullptr;
    other.m_items = inventory_type {};
    other.m_size = 0;

    return *this;
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
typename Inventory<MAX_SIZE, GameObjTy, ItemTy>::inventory_type const& Inventory<MAX_SIZE, GameObjTy, ItemTy>::contents() const
{
    return m_items;
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
typename Inventory<MAX_SIZE, GameObjTy, ItemTy>::inventory_type& Inventory<MAX_SIZE, GameObjTy, ItemTy>::contents()
{
    return m_items;
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
typename Inventory<MAX_SIZE, GameObjTy, ItemTy>::iterator Inventory<MAX_SIZE, GameObjTy, ItemTy>::begin() 
{ 
    return m_items.begin(); 
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
typename Inventory<MAX_SIZE, GameObjTy, ItemTy>::iterator Inventory<MAX_SIZE, GameObjTy, ItemTy>::end()
{
    return m_items.end();
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
typename Inventory<MAX_SIZE, GameObjTy, ItemTy>::const_iterator Inventory<MAX_SIZE, GameObjTy, ItemTy>::cbegin() const 
{ 
    return m_items.cbegin(); 
}
template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
typename Inventory<MAX_SIZE, GameObjTy, ItemTy>::const_iterator Inventory<MAX_SIZE, GameObjTy, ItemTy>::cend() const 
{ 
    return m_items.cend(); 
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
typename Inventory<MAX_SIZE, GameObjTy, ItemTy>::iterator Inventory<MAX_SIZE, GameObjTy, ItemTy>::getItem(std::string_view name)
{
    return m_items.find(name.data());
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
typename Inventory<MAX_SIZE, GameObjTy, ItemTy>::const_iterator Inventory<MAX_SIZE, GameObjTy, ItemTy>::getItem(std::string_view name) const
{
    return m_items.find(name.data());
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
void Inventory<MAX_SIZE, GameObjTy, ItemTy>::useItem(std::string_view name, game_object_pointer target)
{
    useItem(getItem(name), target);
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
template<typename ItemT>
void Inventory<MAX_SIZE, GameObjTy, ItemTy>::addItem(std::string_view id)
{
    if constexpr (!std::is_base_of_v<item_type, ItemT>)
        throw InvalidItemTypeException();

    if (size() >= MAX_SIZE)
    {
        throw InventoryFullException();
    }

    if (m_items.find(id.data()) != m_items.end()) //if we already own this item, increment the count
    {
        if (!m_items[id.data()].first->stackable())
            throw InvalidStackException();
        m_items[id.data()].second += 1; //increment count
        m_size += 1;
    }
    else
    {
        throw InvalidIDException();
    }
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
template<typename ItemT, typename... Args>
void Inventory<MAX_SIZE, GameObjTy, ItemTy>::emplaceItem(std::string_view id, Args... args)
{
    if constexpr (!std::is_base_of_v<item_type, ItemT>)
        throw InvalidItemTypeException();

    if (size() >= MAX_SIZE)
    {
        throw InventoryFullException();
    }

    m_items[id.data()] = std::make_pair(std::make_unique<ItemT>(std::forward<Args>(args)...), 1);
    m_size += 1;
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
void Inventory<MAX_SIZE, GameObjTy, ItemTy>::useItem(iterator pos, game_object_pointer target)
{
    if (pos == m_items.end()) throw InvalidItemException();

    //use the item

    ItemDispatcher<GameObjTy> dispatcher { target };

    auto& it = *pos;
    auto& itemPair = it.second;
    auto& item = itemPair.first;

    if (item->equippable())
    {
        dispatcher.setTarget(m_owner);
        if (!item->is_equipped())
        {
            item->set_equip(true);
            item->use(dispatcher);
        }
        else
        {
            item->set_equip(false);
            item->use(dispatcher);
        }
        return;
    }
    else
    {
        dispatcher.setTarget(target);
        item->use(dispatcher); //dispatcher.target == target, see construction above
    }

    if (!item->reusable())
    {
        if (!item->stackable())
            removeItem(pos);
        else
        {
            if (itemPair.second > 1) itemPair.second -= 1; //decrement count if we have more than 1
            else removeItem(pos);
        }
        m_size -= 1;
    }
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
void Inventory<MAX_SIZE, GameObjTy, ItemTy>::removeItem(iterator pos)
{
    m_items.erase(pos);
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
void Inventory<MAX_SIZE, GameObjTy, ItemTy>::removeItem(std::string_view name)
{
    removeItem(getItem(name));
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
void Inventory<MAX_SIZE, GameObjTy, ItemTy>::setOwner(game_object_pointer owner)
{
    m_owner = owner;
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
typename Inventory<MAX_SIZE, GameObjTy, ItemTy>::game_object_pointer Inventory<MAX_SIZE, GameObjTy, ItemTy>::getOwner() const
{
    return m_owner;
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
typename Inventory<MAX_SIZE, GameObjTy, ItemTy>::size_type Inventory<MAX_SIZE, GameObjTy, ItemTy>::max_size() const
{
    return MAX_SIZE;
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
typename Inventory<MAX_SIZE, GameObjTy, ItemTy>::size_type Inventory<MAX_SIZE, GameObjTy, ItemTy>::size() const
{
    return m_size;
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
template<unsigned int N>
void Inventory<MAX_SIZE, GameObjTy, ItemTy>::merge(Inventory<N, GameObjTy, ItemTy>& other)
{
    if (!merge_fits(other))
        throw InventoryFullException();

    for (auto& it = other.begin(); it != other.end(); std::advance(it, 1))
    {

        this->m_items[it->first] = std::move(it->second);
    }
    other.clear();
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
void Inventory<MAX_SIZE, GameObjTy, ItemTy>::clear()
{
    m_size = 0;
    m_items.clear();
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
template<unsigned int N>
bool Inventory<MAX_SIZE, GameObjTy, ItemTy>::merge_fits(Inventory<N, GameObjTy, ItemTy> const& other)
{
    return !(full() || other.size() + this->size() >= max_size());
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
template<unsigned int N>
void Inventory<MAX_SIZE, GameObjTy, ItemTy>::transfer(Inventory<N, GameObjTy, ItemTy>& destination, std::string_view name)
{   
    if (destination.full())
        return;

    auto& it = getItem(name);
    auto& item = (*it).second;

    destination.contents()[name.data()] = std::move(item);

    m_items.erase(it);
    m_size -= 1;
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
bool Inventory<MAX_SIZE, GameObjTy, ItemTy>::empty() const
{
    return size() <= 0;
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
bool Inventory<MAX_SIZE, GameObjTy, ItemTy>::full() const
{
    return max_size() <= size();
}

template<unsigned int MAX_SIZE, typename GameObjTy, typename ItemTy>
unsigned int Inventory<MAX_SIZE, GameObjTy, ItemTy>::getItemCount(std::string_view id) const
{
    if (m_items.find(id.data()) == m_items.end()) throw InvalidItemException();
    return m_items.at(id.data()).second;
}

I will also add a main.cpp test file just to demonstrate how this can be used

main.cpp

#include "DamagePotion.h"
#include "Inventory.h"
#include "HealPotion.h"
#include "Sword.h"
#include "ItemUtil.h"

std::ostream& operator<<(std::ostream& out, ItemID const& id)
{
    if (id == ItemID::DAMAGE_POTION)
    {
        out << "ID_DAMAGE_POTION";
    }
    else if (id == ItemID::DEFAULT_ITEM) out << "ID_DEFAULT_ITEM";
    else if (id == ItemID::HEAL_POTION) out << "ID_HEAL_POTION";
    else if (id == ItemID::SWORD) out << "ID_SWORD";
    return out;
}

//Replace temp::GameObject class with the GameObject class used by your game

class Player : public temp::GameObject
{
public:
    Player() : temp::GameObject(200)
    {
        try
        {
            m_inventory.emplaceItem<DamagePotion>("Damage Potion I", 50);
            m_inventory.emplaceItem<HealPotion>("Heal Potion I", 70);
            m_inventory.emplaceItem<Sword>("Sword I", 20);

            std::cout << "Inventory contents after adding base items:\n";
            for (auto const& it : m_inventory.contents()) std::cout << it.second.first->id() << "\n";
            std::cout << "\n";

            m_inventory.useItem("Damage Potion I", this);
            m_inventory.useItem("Heal Potion I", this);
            m_inventory.useItem("Sword I");

            std::cout << "Inventory contents after using base items:\n";
            for (auto const& it : m_inventory.contents()) std::cout << it.second.first->id() << "\n";
            std::cout << "\n";

            m_inventory.useItem("Sword I"); // will unequip Sword I

            std::cout << "Inventory contents after unequipping Sword I:\n";
            for (auto const& it : m_inventory.contents()) std::cout << it.second.first->id() << "\n";
            std::cout << "\n";

            chest.emplaceItem<DamagePotion>("CDmgPot", 100);
            chest.emplaceItem<HealPotion>("CHealPot", 200);

            std::cout << "Chest contents after adding base items:\n";
            for (auto const& it : chest.contents()) std::cout << it.second.first->id() << "\n";
            std::cout << "\n";

            m_inventory.merge(chest);

            std::cout << "Chest contents after merging with inventory:\n";
            for (auto const& it : chest.contents()) std::cout << it.second.first->id() << "\n";
            std::cout << "\n";
            std::cout << "Inventory contents after merging with chest:\n";
            for (auto const& it : m_inventory.contents()) std::cout << it.second.first->id() << "\n";
            std::cout << "\n";

            chest.emplaceItem<Sword>("CSword", 50);

            std::cout << "Chest contents after adding CSword:\n";
            for (auto const& it : chest.contents()) std::cout << it.second.first ->id() << "\n";
            std::cout << "\n";

            chest.transfer(m_inventory, "CSword");

            std::cout << "Inventory contents after transferring CSword from chest:\n";
            for (auto const& it : m_inventory.contents()) std::cout << it.second.first->id() << "\n";
            std::cout << "\n";

            chest.emplaceItem<Sword>("CSword", 20);

            std::cout << "Chest contents after adding a CSword:\n";
            for (auto const& it : chest.contents()) std::cout << it.second.first->id() << "\n";
            std::cout << "\n";

            chest.removeItem("CSword");

            std::cout << "Chest contents after removing a CSword:\n";
            for (auto const& it :chest.contents()) std::cout << it.second.first->id() << "\n";
            std::cout << "\n";
        }
        catch (std::runtime_error e)
        {
            std::cerr << e.what();
        }
    }

private:
    Inventory<200> m_inventory { this };

    Inventory<5> chest { this };
};


struct InvalidItem
{

};

struct InvalidEquippable
{
    void equippable()
    {

    }
};

class TestClass : public temp::GameObject
{
public:
    TestClass() : temp::GameObject(50)
    {
        try
        {

            Inventory<100> inv { this };

            inv.emplaceItem<ItemType<ItemID::DAMAGE_POTION>::type>("P", 20);
            inv.addItem<DamagePotion>("P");

            std::cout << ItemName(ItemID::HEAL_POTION) << ' ' << ItemId<Sword>() << '\n';

            std::cout << inv.getItemCount("P") << '\n';

            inv.useItem("P", this);
            inv.useItem("P", this);

            std::cout << getHealth() << '\n';

            std::cout << inv.getItemCount("P") << '\n';
        }
        catch (Inventory<100>::Exception e)
        {
            std::cout << e.what() << "\n";
        }
    }
};



int main()
{

    Player p;


//  IItem* base_ptr;

//  Inventory<200> {nullptr};

    //Fails to compile due to traits
//  Inventory<100, temp::GameObject, InvalidItem> {nullptr};
//  Inventory<100, temp::GameObject, InvalidEquippable> {nullptr};

//  base_ptr = new DamagePotion(20);

//  temp::GameObject target { 100 };
/*
    std::cout << "Using Item: \n";
    std::cout << "Name:\t" << base_ptr->name() << "\n";
    std::cout << "ID:\t" << base_ptr->id() << "\n";
    base_ptr->use(&target);
*/


    std::cin.get();
}

Note: I have just read a meta post that says that it is not allowed to post GitHub links. I am not sure what I should do in this case, as I mostly want the Inventory header reviewed. The files in the github repo are only for when there really is more information required.

EDIT: as per request in the comments, here are some further explanations:

Architecture

As I created this project with portability across other projects in mind, so as a kind of library, the architecture is designed to be as simple to use as possible. All Items will be inherited from a base IItem class, or any other class that supports the functions in the Inventory::Traits class. The inventory is responsible for managing the resources of every item, and for creating them. So when adding an item you call

inventory.emplaceItem<SomePotion>("Potion", 50); //the 50 is passed to SomePotion's constructor

MAX_SIZE

MAX_SIZE is a template parameter due to an unlucky design choice at the very beginning. Since I did not want to do lots of allocations for items, I wanted to have the size fixed. This is probably completely unneccesary, but annoying to remove now.

Item usage

Since the Inventory is responsible for using the items (by calling useItem("name", target_ptr);), I needed some way of doing that. I chose for the Visitor Pattern, which I implemented with the ItemDispatcher class.

Data Structure

The items are stored in a std::unordered_map<std::string, std::pair<std::unique_ptr<Item>, unsigned>>; I will break down why I chose for this. First of all, I wanted the items to be accessible in some way. Iterators were clumsy, because they would invalidate when an item is added to the inventory. I still kept the begin() and end() functions for if you would want to iterate through an inventory in a loop. So I chose for an undoredered map, with string indices so you can give every item its own string ID. The items are stored as a pair of [Item, Count], since I wanted to be able to stack items where possible (Item.stackable() == true).

Reusability

About the reusable in multiple projects. With that I mean that if I, at some point decide to make an RPG game, and the character needs an inventory, that I can just use this exact Inventory class without any modifications. If I then later want to make another, completely different game that also features an Inventory, or some other way of storing items, I can reuse this again.

Techniques

The Traits class is used as a wrapper for the SFINAE type traits that I use to detect if an eventual custom Item type supports all required functions, set for example like:

Inventory<200, MyCoolGameObject, MyCoolItem> inventory;

Traits is a class because I can't do

class X
{
    namespace Y
    {

    };
};

You might get more feedback if you explain the techniques you are using (e.g. Traits class), and summarize the architecture. — JDługosz
– JDługosz, Commented May 16, 2018 at 20:34
I agree with the comment above that you should describe the general architecture given the complexity of your code. I think you should also discuss alternative choices you could have made and why you settled on some choices in the end. Discussing the use case is also important. You say "it should be reusable in multiple projects", but I'm not sure what it implies. The code seems way over-complicated. One small example: why is MAX_SIZE in the type signature and not just a member variable of Inventory? — toto2
– toto2, Commented May 17, 2018 at 1:49
Out of curiosity, did all that meta-template programming actually help you in any way? Did you actually get any noticeable benefits over using a polymorphic class that had a bunch of pure virtual functions (use(), equippable(), is_equipped(), set_equip(), unequip(), etc)? — red_eight
– red_eight, Commented May 18, 2018 at 4:06
_Ty is an _Ugly format name. Those are reserved for use by the library implementation. See timsong-cpp.github.io/cppwp/n4659/lex.name#3 . Writing your this in a way that mimics what you see in the compiler-supplied headers is exactly wrong! — JDługosz
– JDługosz, Commented May 18, 2018 at 6:13
@MivVG As long as there are no answers yet, you may still modify the question. That rule's intention is to not invalidate existing answers. — Roland Illig
– Roland Illig, Commented May 21, 2018 at 8:46

gyre · Accepted Answer · 2018-05-28 02:21:12Z

I'm full of opinions, so take this all with a grain of salt, but I hope some of it proves helpful. Let me know if you have any questions after reading this.

Naming Conventions:

Identifiers with a leading underscore like _Ty are sometimes reserved, according to the rules detailed in this answer; so prefer the trailing underscore or omit them entirely.
For the most part, you named your template parameters descriptively, which is much better for readability than single-letter names like T, U, etc. (though these are perfectly acceptable when there is a single parameter or the meaning is otherwise self-explanatory). That said, I don't think the Ty suffix is particularly attractive, so I would put forth this alternative convention:
```
template <bool ExampleNonTypeParameter, typename ExampleTypeParameterT>
```
(In your case this would mean using MaxSize and ItemT in place of MAX_SIZE (all-caps screams MACRO_MAGIC_IS_AT_PLAY to my eye) and ItemTy.
Abbreviate a name consistently: everywhere or not at all. GameObjTy might as well be GameObjectT, since you've already established the name temp::GameObject.
Speaking of GameObject, perhaps consider renaming this to Object or Entity and letting the namespace name do the work of disambiguating, e.g. my_game_engine::Object from some other meaning of 'object'.
Plenty of people follow the m_member_variable convention, but consider avoiding the extra noise with simply member_variable or member_variable_ (note the trailing underscore). Whatever way you end up leaning, get rid of this-> (you only did this twice, but ideally you should only use this by itself or with a preceding * ).
Choose either camelCase or snake_case for function and variable names. Don't mix the two. It's hard enough learning an interface without having to remember which names use a particular casing convention. I recommend snake case simply because that's the way the standard template library and most new C++ standard library interfaces are written.
Function names should almost always be verb phrases. The only exceptions I can think of are 'getters', in which the leading get_ is often dropped (for example, containers use size() rather than get_size()). Even though the STL uses empty(), it's ambiguous whether this name is a verb or a noun, and I think a better convention would have been is_empty() (and is_full() for consistency, in the case of your Inventory class).

Interface:

The ItemDispatcher feels like overengineering to me. You mention that "at first [use] was a virtual function, but someone told me that using a visitor pattern for that was a good choice." I disagree, based on this answer explaining when to use the visitor design pattern. I expect that you're far more likely to add a new item, with accompanying new behavior, than a new 'operation' that applies to all (including existing) items, in which case the coupling between an item and its use algorithm is desirable. An example of the latter would be a new action method, e.g. throw_at(GameObject &). I recommend:
```
virtual void use_on(GameObject & target) = 0;
```
Pointers, pointers everywhere! Don't forget that pointers aren't the only way to avoid object slicing of polymorphic classes; references work equally well. You've already learned about std::unique_ptr, which is wonderful. Everywhere the concept of object 'ownership' is irrelevant, prefer a reference to a raw pointer. If the owner is optional, document that by using std::optional<std::reference_wrapper<GameObject>> in its place.
set_equip(bool) could be split into equip() and unequip() methods.
'Equipable' (note only one 'p') is pseudo-slang in the gaming community, and not exactly a real word. If that bothers you, feel free to use can_be_equipped instead.
You can omit the other parameter name when deleting Inventory's copy constructor and copy assignment operator:
```
Inventory(Inventory const&) = delete;
Inventory & operator=(Inventory const&) = delete;
```
I don't see any concrete advantages to locking in the maximum size of an inventory at compile time via a template parameter. You're not using any data structures like std::array where constexpr sizing would be required. I think it belongs as a private field, and capacity() might be a better name in case you do end up making the value change throughout the game (for example, a magic item might make a target player's inventory grow in capacity as a fun mechanic).
Currently, Inventory exposes its internal std::unordered_map via the contents() accessor. Since begin(), end(), et al. are already provided as a means for enumerating the contained items, encapsulate those implementation details. As an added benefit, removing contents() is more consistent with the idea of an inventory being a kind of container itself (rather than something that 'contains' a container).
Inventory::Traits is templated based on an item type, which is counter-intuitive. Perhaps this could have been called ItemTraits (but I'll explain why I don't think that class is necessary at all later).
It's also odd that Inventory::inventory_type refers to the type of the internal map... maybe use item_container_type or something similar.
After poring over the code, I still don't feel like I have a great mental model of what your inventory is supposed to 'look and feel' like. Is it a bag; do you reach in and pull things out? Is it a grid with specific slots? Is it going to be searchable? I think you will be able to find a better data structure if you can describe a real-life analogy for it or at least draw how you hope it will be displayed and interacted with on screen.
The noexcept specifier is your friend; use it to decorate accessors like is_empty(), is_full(), size(), etc.
Speaking of exceptions, you throw a lot of them! Make sure custom ones inherit from std::runtime_error. It's good to throw exceptions in exceptional situations, but I think you could remake your Inventory interface to work largely without them...
For starters, everywhere you wrote if constexpr (...) throw ... should be replaced with static_assert(/* boolean expression */), which will prevent the program from compiling if that invariant doesn't hold.
For manipulating items within the inventory, here's an alternative interface (here I'm using Slot as a substitute for std::string, but I hope you'll consider making it a real type in the future if that is indeed what you were trying to represent):
```
SizeT count() const;
SizeT count(Slot slot) const;
bool try_insert(Slot slot, ItemT const& item, SizeT count = 1);
bool try_remove(Slot slot, ItemT const& item, SizeT count = 1);
bool try_merge(Inventory & other);
void clear(Slot slot);
void clear() noexcept;
```
Note the symmetry between insertion and removal; each default to working with a single item (your existing solution always adds only one item but removes all items, which conceptually I think is better termed 'clearing a slot'). Each of the try_ functions returns a boolean representing whether the operation succeeded fully or not; if it failed, the inventory is left unchanged. This avoids the stackability- and fullness-related exceptions.

Implementation:

Inside empty(), return size() <= 0; really throws me off. I immediately think: whoa, how could size ever be negative? I sure hope it's an unsigned type! It's much less surprising to read return size() == 0;.
The Yoda conditional in full() should be changed to return size() >= max_size();. Ideally, your interface would ensure that size() <= max_size(), making the above equivalent to an equality comparison, but just in case there's a bug it's better not to accidentally report that an already over-filled inventory is still 'not full'.
merge_fits could have just been return size() + other.size() < max_size();
The whole jumble of SFINAE and traits at the beginning of Inventory can be replaced with an abstract Item base class (with the pure virtual methods you were trying to detect originally), from which all items must descend, and a single static assertion:
```
 template <class ItemT>
 class Inventory {
   static_assert(std::is_base_of_v<Item, ItemT>());           
   ...
```
I won't go further into specific code simplifications right now, because I think refitting the interface will make many of those changes obsolete. Feel free to comment with any later iterations of the code (and specific sections to examine) and I can narrow down my focus.

This is great. Thanks a lot for taking your time to write this review. I will be working on the changes you proposed, and I will post here when I think I'm done. — MivVG
– MivVG, Commented May 28, 2018 at 14:40

Stack Exchange Network

C++ Inventory System

1 Answer 1

Naming Conventions:

Interface:

Implementation:

You must log in to answer this question.

Linked

Hot Network Questions

C++ Inventory System

1 Answer 1

Naming Conventions:

Interface:

Implementation:

You must log in to answer this question.

Linked

Related

Hot Network Questions