Still having an Unhandled Exception after editing it a bit

Question

Inventory::Inventory()
{
    this->cap = 10;
    this->nrOfItems = 0;
    this->itemArr = new Item* [cap]();
}


Inventory::~Inventory()
{
    for (size_t i = 0; i < this->nrOfItems; i++)
    {
        delete this->itemArr[i];
    }
    delete[] this->itemArr;
}

void Inventory::expand()
{
    this->cap *= 2;

    Item **tempArr = new Item*[this->cap]();



    for (size_t i = 0; i < this->nrOfItems, i++;)
    {
        tempArr[i] = new Item(*this->itemArr[i]);
    }

    for (size_t i = 0; i < this->nrOfItems, i++;)
    {
        delete this->itemArr[i];
    }
    delete[] this->itemArr;


    this->itemArr = tempArr;

    this->initialize(this->nrOfItems);
}

void Inventory::initialize(const int from)
{
    for (size_t i = from; i < cap, i++;)
    {
        this->itemArr[i] = nullptr;
    }
}

void Inventory::addItem(const Item& item)
{
    if (this->nrOfItems >= this->cap)
    {
        expand();
    }

    this->itemArr[this->nrOfItems++] = new Item(item);
}

void Inventory::removeItem(int index)
{

}

Above is my code from Inventory.cpp and the issue is that I keep getting an Unhandled Exception from the line that has:

this->itemArr[i] = nullptr;

I have no idea where I'm messing up in the code. Below I am posting from Inventory.h:

class Inventory
{
private:
    int cap;
    int nrOfItems;
    Item **itemArr;
    void expand();
    void initialize(const int from);

public:
    Inventory();
    ~Inventory();
    void addItem(const Item &item);
    void removeItem(int index);
    inline void debugPrint() const    
    {
        for (size_t i = 0; i < this->nrOfItems; i++)
        {
            std::cout << this->itemArr[i]->debugPrint() << std::endl;
        }
    }

};

This is where itemArr should be housed but for some reason, it is not pulling. I am new to coding so I don't know all of the tips and tricks involved with it.

I can't be 100% sure, but more likely this is a dupe of: https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three — NathanOliver, Nov 05 '20 at 21:01
What is the value of `i` at the time of the problem? What is the size of the relevant array at that time? Similarly what about `cap` and `nrOfItems` at that moment? Is this when `initialize` is being called from `expand` or do you call it elsewhere? In fact, if it's initializing things, why aren't you calling it from the constructor after `this->itemArr = new Item* [cap]();` ? — TheUndeadFish, Nov 05 '20 at 21:02
@TheUndeadFish there is no need to call `initialize()` in the `Inventory` constructor, as the `()` after `[cap]` will directly value-initialize the array elements to `nullptr`. In fact, the new array in `expand()` is also being value-initialized, too, making the call to `initialize()` redundant. — Remy Lebeau, Nov 05 '20 at 21:06
Prefer to use a `std::vector>` over this two- star programming. — JHBonarius, Nov 05 '20 at 21:08
I suspect that `Item` has subclasses, and that you're going to be surprised by *object slicing* soon. If it doesn't, there's no point in having an array of pointers. — molbdnilo, Nov 05 '20 at 22:27

score 2 · Answer 1 · answered Nov 05 '20 at 21:16

In the loop i < cap, i++;, you will first check if i is in bounds, and then increase it by one before the loop body executes. Thus if i=cap-1 the check will pass, i will become cap and you're out of bounds. Instead write (size_t i = from; i < cap;i++), so that the increment of i is executed after the loop body.

Remy Lebeau · Answer 2 · 2020-11-05T21:50:28.193

A see several problems with your code:

In Inventory::expand(), since you are working with an array of pointers (why not an array of objects?), there is no need to make clones of the existing Item objects to the new array, just copy the existing pointers as-is. You are just expanding the array to fit more pointers, so the clones are wasted overhead. Not a fatal issue, but it is something you should be aware of.
In both Inventory::initialize() and Inventory::expand(), your loops are setup incorrectly. You are performing the i++ in the wrong place. Given this definition of a loop:

for ( <init-statement> <condition> ; <iteration_expression> )

You are performing the i++ as part of the <condition>, not the <iteration_expression>. A simply typo caused by you using the comma operator instead of ;, but an important typo nonetheless.
Inventory is violating the Rule of 3/5/0, by not implementing copy/move constructors and copy.move assignment operators.

With that said, try this instead:

class Inventory
{
private:
    size_t cap;
    size_t nrOfItems;
    Item **itemArr;

    void expand();
    
public:
    Inventory();
    Inventory(const Inventory &src);
    Inventory(Inventory &&src);
    ~Inventory();

    Inventory& operator=(Inventory rhs);

    void addItem(const Item &item);
    void removeItem(size_t index);

    inline void debugPrint() const    
    {
        for (size_t i = 0; i < nrOfItems; ++i)
        {
            std::cout << itemArr[i]->debugPrint() << std::endl;
        }
    }
};

Inventory::Inventory()
{
    cap = 10;
    nrOfItems = 0;
    itemArr = new Item*[cap]();
}

Inventory::Inventory(const Inventory &src)
{
    cap = src.cap;
    nrOfItems = src.nrOfItems;
    itemArr = new Item*[cap]();
    for(size_t i = 0; i < nrOfItems; ++i)
    {
        itemArr[i] = new Item(*(src.itemArr[i]));
    }
}

Inventory::Inventory(Inventory &&src)
{
    cap = src.cap; src.cap = 0;
    nrOfItems = src.nrOfItems; src.nrOfItems = 0;
    itemArr = src.itemArr; src.itemArr = nullptr;
}

Inventory::~Inventory()
{
    for (size_t i = 0; i < nrOfItems; ++i)
    {
        delete itemArr[i];
    }
    delete[] itemArr;
}

Inventory& Inventory::operator=(Inventory rhs)
{
    Inventory temp(std::move(rhs));
    std::swap(cap, temp.cap);
    std::swap(nrOfItems, temp.nrOfItems);
    std::swap(itemArr, temp.itemArr);
    return *this;
}

void Inventory::expand()
{
    size_t newCap = cap * 2;
    Item **tempArr = new Item*[newCap]();

    for (size_t i = 0; i < nrOfItems; ++i)
    {
        tempArr[i] = itemArr[i];
    }

    delete[] itemArr;

    itemArr = tempArr;
    cap = newCap;
}

void Inventory::addItem(const Item& item)
{
    if (nrOfItems >= cap)
    {
        expand();
    }

    itemArr[nrOfItems] = new Item(item);
    ++nrOfItems;
}

void Inventory::removeItem(size_t index)
{
    if (index < nrOfItems)
    {
        Item *item = itemArr[index];

        for(size_t i = index + 1; i < nrOfItems; ++i)
        {
            itemArr[i-1] = itemArr[i];
        }

        --nrOfItems;
        itemArr[nrOfItems] = nullptr;

        delete item;
    }
}

That being said, you really should be using std::vector instead, let it handle these details for you, eg:

#include <vector>

class Inventory
{
private:
    std::vector<Item> itemVec;
    
public:
    Inventory();

    void addItem(const Item &item);
    void removeItem(size_t index);

    inline void debugPrint() const    
    {
        for (Item &item : items)
        {
            std::cout << item.debugPrint() << std::endl;
        }
    }
};

Inventory::Inventory()
{
    itemVec.reserve(10);
}

void Inventory::addItem(const Item& item)
{
    itemVec.emplace_back(item);
}

void Inventory::removeItem(size_t index)
{
    if (index < itemVec.size())
    {
        itemVec.erase(itemVec.begin() + index);
    }
}

See how much simpler that is? :)

Still having an Unhandled Exception after editing it a bit

2 Answers2