Interfacing functions in one class from another

[Ch1156]

So Im working on a very old project, I decided to go back and update the code and look it over to see what i can chnage and apply new knowledge to it, and I use chat GPT 4 for guidance(with a grain of salt of course). I was telling it that I have an inventory class that operates separately from everything else, even though characters have inventories, i instantiate inventory objects for each character, and the AI suggested adding Inventory to Character and adding an interface to it to allow for all characters that derive from the character class to perform operations on it, is this good practice? it says it is but idk for sure, just thought id ask. It seems weird to me to do that but the AI makes a good point but I thought id ask an actual person.

The AI says:

It's generally a good idea to separate concerns and have dedicated classes for specific purposes. In your case, having an Inventory class is a good design choice, as it keeps the inventory logic separate from the characters.

However, it's important that the Inventory class is integrated properly with the other classes, such as Character, Player, and Enemy. Instead of instantiating the Inventory class on its own, consider having an Inventory object as a member variable within the Character class (or within both Player and Enemy classes, if they don't inherit from a common Character class). This way, you can manage the inventory separately for each character, and the inventory management logic will be encapsulated within the Inventory class.

Inventory.h

// This is an independent project of an individual developer. Dear PVS-Studio, please check it.

// PVS-Studio Static Code Analyzer for C, C++, C#, and Java: http://www.viva64.com

#ifndef INVENTORY_H
#define INVENTORY_H
#pragma once

#include <iostream>
#include <string>
#include <vector>
#include <limits>
#include <utility>
#include <iterator>

#include "Item.h"

class Player;

class Inventory
{
	public:
		Inventory() = default;

		void Add(Item& item, int amount);
		void Open();
		void Clear();
		void UseItem(Player& Hero);

		std::vector<std::pair<Item, int>>& GetInventory();

	private:
		std::vector<std::pair<Item, int>> mInventory{ 0 };
		int mItemsOwned{ 0 };
		Item item;


		void IncrementItemsOwned(int amount, int index);
		void DecrementItemsOwned(int amount, int index);
};

#endif 

Character.h

// This is an independent project of an individual developer. Dear PVS-Studio, please check it.

// PVS-Studio Static Code Analyzer for C, C++, C#, and Java: http://www.viva64.com

#ifndef CHARACTER_H
#define CHARACTER_H
#pragma once

#include <iostream>
#include <string>
#include <ostream>
#include <vector>

#include "Item.h"
#include "Attack.h"
#include "Inventory.h"

class Enemy;

class Character
{
    public:
        std::string GetName() const;
        int GetHealth() const;
        int GetMaxHealth() const;
		void ResetHealth();
		void Heal(int amount);
		void GiveMoney(int amount);
        int GetMoney() const;

        void AddItemToInventory(Item& item, int amount);
        void OpenInventory();
        void ClearInventory();
        void UseInventoryItem();
        std::vector<std::pair<Item, int>>& GetInventory();

        void TakeDamage(int amount);

        void GiveAttack(Attack& attack);
		std::vector<Attack>& GetAttackList();

        friend std::ostream& operator<<(std::ostream& os, const Character& character);

    //Protected because we dont want the user to create Characters, we want them to create players and enemies.
    protected:
        Character
        (
            const std::string& name, 
            int health, 
            const int maxHealth,
            int money
        ) :     mName(name), 
                mHealth(health),
                MAX_HEALTH(maxHealth),
                mAttackList(0),
                mMoney(money)
        {}
        Character() = default;

		std::string mName{ "Character Name" };

    protected:
        Inventory mInventory;

    private:
        int mHealth{ 100 };
        std::vector<Attack> mAttackList{ 0 };
        Item mItem;
        int mMoney{ 10 };

		const int MAX_HEALTH{ 100 };
};

#endif 

[AbstractionAnon]

Instead of instantiating the Inventory class on its own, consider having an Inventory object as a member variable within the Character class

What the AI is telling you is basically sound advice. Ask yourself if each Character "HAS-A" inventory. If the answer to that is yes, then each character should have an Inventory as a member variable.

I'm not keen on the structure of the inventory class though.
std::vector<std::pair<Item, int>> mInventory{ 0 };
That structure requires that you know the position of an Item within the inventory vector. A std::map may make more sense:
std::map<Item,int> mInventory{};

[Peter87]

std::vector could be used if order is important.

[Ch1156]

Yeah the composition part makes total sense, A character has-an inventory, its the functions thats only purpose is to just call other functions that is a little weird to me, but maybe this is totally acceptable, im not sure. Im basically just re defining the same functions in a different class that calls those same functions. as for using a map, yeah a map could work, but i think there was a specific reason i used a vector, i cant remember too well. I think i chose vector because I didnt want the items to be auto sorted, I wanted to have control over the sorting. I maybe could have used an unordered_map but i decided to stick with vector.

[seeplus]

A map can be stored sorted in any defined key order - just provide a comparison functor. The default is std::less
https://en.cppreference.com/w/cpp/container/map

[Ch1156]

I'll have to take a look at that and play around with it, i always thought maps couldnt be sorted by the user.

[Peter87]

seeplus wrote:

A map can be stored sorted in any defined key order

True, but not very helpful if they are not ordered by "key". You might for example want the items in the order that they were added to the inventory.

Another limitation of using Item as map key is that they cannot be modified. This might or might not be an issue.

[kigar64551]

its the functions thats only purpose is to just call other functions that is a little weird to me, but maybe this is totally acceptable, im not sure. Im basically just re defining the same functions in a different class that calls those same functions.

I think the idea is to hide the implementation details (and some possible complexity) in the Inventory class. Maybe, at this time, the methods of your Inventory class are just thin wrappers around std::vector or std::map, but the users of your Inventory class don't have to know or care! They just use the public Inventory interface, unknowingly how it is implemented internally. Maybe, at a later time, you'll decide that the Inventory class should maintain the items in a different way, e.g. you may want to use a std::multimap from now on. This then would be a change internal to the Inventory class. Users of the existing Inventory interface don't need to be updated!

If, instead, all entities that need to maintain an inventory would be using std::vector or std::map directly, then changing the way how an inventory is implemented becomes much harder, because it would require changes in a lot of different places...

Also, you may want to add some "helper" methods to operate on the inventory. For example, removeAllBrokenItems() or whatever. The Inventory class would be the perfect place to implement such "helper" methods at a single sensible place. Without a dedicated Inventory class, however, such methods would have to be in the class that uses the inventory (e.g. in the Character class), which is not a good separation of duties. Even worse, if the "helper" method is required by more than one class that works with an inventory, then you'd have to create multiple redundant copies of the "helper" method – one in each class that works with the inventory. But, redundant code always is a bad idea! So, I think it will be much better to encapsulate all inventory-related methods in its own dedicated class.

In general, it is always good to break down your program into small self-contained units which communicated via well-defined interfaces. The "internals" of such units may change over time, but the interface should remain stable, or at least only be "extended" in backwards-compatible ways. This way, you can easily change/improve your implementation without having to worry that it will break something...

[seeplus]

True, but not very helpful if they are not ordered by "key". You might for example want the items in the order that they were added to the inventory.

A key can be a struct that includes those elements needed for a required sort order. Of course you can only specify 1 sort order. If you need multiple sort orders then there are other ways (eg having other containers of pointers).

[JLBorges]

O(1) look up on key; and iteration in insertion order.
As in https://github.com/Tessil/ordered-map

[Peter87]

seeplus wrote:

A key can be a struct that includes those elements needed for a required sort order.

Yes, but isn't the whole point of using a map that you can find the items easily and efficiently? If you add additional things to the key just to get the order you want then you can no longer use the find lookup function to look for items without knowing these additional things about the item. Instead you would have to iterate over the elements and look for the item that you're searching for.

Iterating over all the elements should be faster with std::vector compared to std::map.

In this case it might not matter too much performance-wise whether you can use map::find or have to iterate over a map or vector because the number of elements are probably small.


We don't even know what "Item" is and whether lookup on it makes sense. Maybe Item is a complicated class type. Maybe you can have identical items at two different "slots". Maybe the inventory is searched for certain properties (e.g. find the first drinking potion [healing, mana, etc.]) rather than exact matches. The information given in this thread is not enough to tell.