Searching for duplicate vector items

[Ch1156]

I need to be able to search my vector so that duplicate items arent added. So If a potion is added again, it will just add the amount instead of the item again. I feel like im close but cant seem to figure out whats wrong. I created a whole new program just to test out the search feature, complete with classes and all that, wat am i doing wrong?

problem code

int wordAppearance{ -1 };
    int index{ 0 };

	for (int position = item.GetName().find(itemName, 0); position != string::npos; position = item.GetName().find(itemName, position))
	{
		cout << "Found " << ++wordAppearance << " instances of " << itemName << " at position " << position << endl;
		position++;
	}
    if (wordAppearance == 0)
    {
		mInventory.push_back(std::make_pair(item.GetName(), amount));
    }
	else
    {
        cout << "Item already exists" << endl;
        item.IncrementAmountOwned(amount);
    }

Full code

// 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


#include <iostream>
#include <vector>
#include <string>
#include <algorithm>

using std::cout;
using std::endl;
using std::vector;
using std::string;
using std::sort;
using std::unique;

class Item
{
    public:
        Item(const string& name): mName(name), mAmountOwned(0)
        {}
		Item() = default;

        string GetName() const { return mName; }
        int GetAmountOwned() const { return mAmountOwned; }
		void IncrementAmountOwned(int amount);

    private:
        string mName{ "Item Name" };
        int mAmountOwned{ 0 };
};

void Item::IncrementAmountOwned(int amount)
{
    mAmountOwned += amount;
}

class Inventory
{
    public:
        Inventory(): mInventory(0)
        {}

        void Add(Item& item, const string& itemName, int amount);
        void Open();

    private:
        vector <std::pair<string, int>> mInventory;
        Item item;
};



void Inventory::Add(Item& item, const string& itemName, int amount)
{
    int wordAppearance{ -1 };
    int index{ 0 };

	for (int position = item.GetName().find(itemName, 0); position != string::npos; position = item.GetName().find(itemName, position))
	{
		cout << "Found " << ++wordAppearance << " instances of " << itemName << " at position " << position << endl;
		position++;
	}
    if (wordAppearance == 0)
    {
		mInventory.push_back(std::make_pair(item.GetName(), amount));
    }
	else
    {
        cout << "Item already exists" << endl;
        item.IncrementAmountOwned(amount);
    }
}

void Inventory::Open()
{
    for (auto& i : mInventory)
    {
        cout << i.first << " " << i.second << endl;
    }
}

int main()
{
    vector<string> inv;
    
    inv.push_back("Potion");
    inv.push_back("Map");

    Item Potion("Potion");
    Item Map("Map");

    Inventory PlayerInventory;

    PlayerInventory.Add(Potion, Potion.GetName(),  3);
    PlayerInventory.Add(Map, Map.GetName(), 1);

	PlayerInventory.Add(Potion, Potion.GetName(),  3);
	PlayerInventory.Add(Potion, Potion.GetName(),  3);
	PlayerInventory.Add(Potion, Potion.GetName(),  3);


    PlayerInventory.Open();
}

[dhayden]

#include <map>
#include <string>
#include <iostream>

using std::map;
using std::string;
using std::cout;

class Inventory: public map<string, int> {
public:
    void Open();
};

void
Inventory::Open()
{
    for (auto &item : *this) {
	cout << item.first << ' ' << item.second << '\n';
    }
}

int main()
{
    Inventory PlayerInventory;
    PlayerInventory["Potion"] += 3;
    PlayerInventory["Map"] += 1;
    PlayerInventory["Potion"] += 3;
    PlayerInventory["Potion"] += 3;
    PlayerInventory["Potion"] += 3;

    PlayerInventory.Open();
}

Output:

Map 1
Potion 12

Here are the problems with the way you've done it.

The quantity (mAmountOwned) isn't a property of the item, it's a property of the inventory. Think about it in the real world. A chair is wood or metal, it has a color and a height. It does NOT have a quantity. The room it sits in has a quantity of chairs. The warehouse has a quantity of chairs. But the chair itself doesn't have a quantity.

In my opinion, IncrementAmountOwned is unnecessary and just gets in the way. Many people here will disagree though.

Why does an inventory have a single item? You'd expect it to have a collection of items. Instead it has a collection of pairs where each pair has the same data members as an item. This is very unintuitive and should raise a red flag that you're representing the data wrong.

Inventory::Add() takes an item and a name and a quantity? So can I do this?

Item dog("Dog");
Inventory myInventory;
myInventory.Add(dog, "Cat", 7);

One way to catch problems like this is to add comments explaining the parameters to each method/function. If you can't explain what they are, then it's good sign that you have something wrong.

Also the implementation of Add() seems odd and when I run your program, it clearly doesn't work correctly because it's allowing duplicates. It searches the item parameter for the name. That suggests that the Item's name should actually be a list of names. Since you initialize wordAppearance to -1, the if at line 65 will be true if the name is found exactly once in the item's name, not zero times as the code suggests. Also if you add it, you push_back() the item's name (the string of all items) rather than the name parameter, which is the one name that you searched for.

[Ch1156]

Thank you, I was thinking of using a map, because of its ability to not accept duplicate items but ive been told to always use a vector for game inventories, but idk. The only reason I didnt want to use a map is because it automatically sorts items by name and I want full control on if an item is sorted or not.

[Ch1156]

What does

class Inventory: public map<string, int> {

do? I've never seen a variable or stl container declared like that in the class inheritance access specifier area, cant remember the exact term for it. Also what is the best stl container for a game inventory, is it the map? I always see people saying use a vector which is why i did it that way.

[thmm]

I think it depends what Inventory is doing. I guess it just hold a bunch of Items, probably not more than a few dozens. A vector should be fine because it has several advantages.
You can easily sort it anyway you want. It has contiguos memory, few allocations and is
cach-friendly

[Ch1156]

Well for this game it just needs to hold the item object. I'm unsure what circumstances i would use a vector over a map or vice versa. I need to be able to check and see if the vector holds the same item, and if it does then just increment the already existing one, which is the advantage a map has over a vector, i wish there was an inbetween,.

[seeplus]

What does

class Inventory: public map<string, int> {

do?

Inventory is just a derived class from std::map<string, int> - just as you would derive a new class from another class.

The problem in doing things like this with polymorphism is that the std::containers et al don't have a virtual destructor.

[Ch1156]

Thank you for the additional responses thmm and seeplus. I have modified my code and opted to keep the vector and the increment class members, however I still cannot figure out how to check and see if the vector already contains the item already. I know how to use find with a vector to find a string or a number but i cant figure out how to compare what I have found with what i want to add and then either increment the amount owned if it exists or add it and an amount if it doesn't. I have found some solutions online but they seem a bit complicated, and maybe an operation like that is, I have no idea.

This is the only way I know how to search a string, which is code i copy pasted from a string example project but i cant get it working with this vector, and Im not sure if this code is out of date or if there is a better c++11 and beyond alternative.

 int wordAppearance{ -1 };
    int index{ 0 };

	for (int position = item.GetName().find(itemName, 0); position != string::npos; position = item.GetName().find(itemName, position))
	{
		cout << "Found " << ++wordAppearance << " instances of " << itemName << " at position " << position << endl;
		position++;
	}

Here is my new code:

// 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

#include <iostream>
#include <vector>
#include <string>
#include <algorithm>

using std::cout;
using std::endl;
using std::vector;
using std::string;
using std::sort;
using std::unique;

class Item
{
    public:
        Item(const string& name): mName(name)
        {}
		Item() = default;

        string GetName() const { return mName; }

    private:
        string mName{ "Item Name" };
};

class Inventory
{
    public:
        Inventory(): mInventory(0), mAmountOwned(0)
        {}

        void Add(Item& item, int amount);
        void Open();

		int GetAmountOwned() const { return mAmountOwned; }
		void IncrementAmountOwned(int amount);

    private:
        vector <Item> mInventory;
		int mAmountOwned{ 0 };

};

void Inventory::IncrementAmountOwned(int amount)
{
    mAmountOwned += amount;
}


void Inventory::Add(Item& item, int amount)
{
    //Check and see if item already exists inside vector
    //If item exists, then increment amount owned
    //if item doesnt exist, then add item and add amount specified.
}

void Inventory::Open()
{
    for (auto& i : mInventory)
    {
        cout << i.GetName() << endl;
    }
}

int main()
{
    vector<string> inv;
    
    inv.push_back("Potion");
    inv.push_back("Map");

    Item Potion("Potion");
    Item Map("Map");

    Inventory PlayerInventory;

    PlayerInventory.Add(Potion, 0);
    PlayerInventory.Add(Map, 0);

	PlayerInventory.Add(Potion, 0);
	PlayerInventory.Add(Potion, 0);
	PlayerInventory.Add(Potion, 0);


    PlayerInventory.Open();
}

[thmm]

To find an Item in the vector you can use std::find with an overloaded operator == for Item, or with std::find_if with a lambda.

[Ch1156]

So I was able to figure it out, now the issue of incrementing the amount of items owned for a particular item. So could I just do:

vector<pair<Item, int>> mInventory?

As dhayden pointed out:

Why does an inventory have a single item? You'd expect it to have a collection of items. Instead it has a collection of pairs where each pair has the same data members as an item.

I made a mistake in making the vector of pairs have a string instead of an item and an int.

This is the current working code, is there any more issues with it?

// 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

#include <iostream>
#include <vector>
#include <string>
#include <algorithm>
#include <iterator>

using std::cout;
using std::endl;
using std::vector;
using std::string;
using std::sort;
using std::unique;
using std::distance;
using std::find;

class Item
{
    public:
        Item(const string& name): mName(name)
        {}
		Item() = default;

        string GetName() const { return mName; }

    private:
        string mName{ "Item Name" };
};

bool operator==(const Item& lhs, const Item& rhs)
{
    return lhs.GetName() == rhs.GetName();
}

bool operator!=(const Item& lhs, const Item& rhs)
{
    return !(lhs == rhs);
}

class Inventory
{
    public:
        Inventory(): mInventory(0), mAmountOwned(0)
        {}

        void Add(Item& item, int amount);
        void Open();

		int GetAmountOwned() const { return mAmountOwned; }
		void IncrementAmountOwned(int amount);

    private:
        vector <Item> mInventory;
		int mAmountOwned{ 0 };
};


void Inventory::IncrementAmountOwned(int amount)
{
    mAmountOwned += amount;
}


void Inventory::Add(Item& item, int amount)
{
    vector<Item>::iterator itr = find(mInventory.begin(), mInventory.end(), item.GetName());

    int index = distance(mInventory.begin(), itr);

    if (itr != mInventory.end())
    {
        cout << "Found " << item.GetName() << " in vector, at index: " << index << " which is the #" << index + 1 << " spot in the vector." << endl;
    }
    else
    {
        cout << "Did not find " << item.GetName() << " in vector, so it was added." << endl;
        mInventory.push_back(item);
    }
}

void Inventory::Open()
{
    cout << "\nInventory\n" << endl;

    for (auto& i : mInventory)
    {
        cout << i.GetName() << " (" << mAmountOwned <<")" << endl;
    }
}

int main()
{
    vector<string> inv;
    
    inv.push_back("Potion");
    inv.push_back("Map");

    Item Potion("Potion");
    Item Map("Map");

    Inventory PlayerInventory;

    PlayerInventory.Add(Potion, 1);
    PlayerInventory.Add(Map, 1);

	PlayerInventory.Add(Potion, 1);
	PlayerInventory.Add(Potion, 1);
	PlayerInventory.Add(Potion, 1);


    PlayerInventory.Open();
}

[thmm]

In VS 2019 I get these 2 warnings.

Forum_CPP.cpp(65,37): warning C4100: 'amount': unreferenced formal parameter
Forum_CPP.cpp(65,37): warning G40510A9C: unused parameter 'amount' [clang-diagnostic-unused-parameter]
void Inventory::Add(Item& item, int amount)

If you don't use amount then remove it. What is int mAmountOwned actually for ?

On line 69 and 71 I would rather use auto, also is not the proper type for distance. distance return a ptrdiff_t

[seeplus]

As C++17 with VS2019:

void Inventory::Add(const Item& item, int)
{
	if (const itr {find(mInventory.cbegin(), mInventory.cend(), item.GetName())}; itr != mInventory.cend()) {
		const auto index {distance(mInventory.cbegin(), itr)};

		cout << "Found " << item.GetName() << " in vector, at index: " << index << " which is the #" << index + 1 << " spot in the vector.\n";
	} else {
		cout << "Did not find " << item.GetName() << " in vector, so it was added.\n";
		mInventory.push_back(item);
	}
}

[dhayden]

Class Item adds no value over string (or maybe const string). I'd get rid of it.

"Amount" is a property of the items in the inventory. Sorry, I wasn't clear about that before. In other words, each item in an inventory has an amount associated with it (the number or amount of that item in that inventory).

As I said before, a std::map does everything you want to do here.

Dave

[Ch1156]

Awesome i got it working, thank you all for your help.