Better way of doing this?

[danielmccarthy]

I am wondering if their is a better way to do this

void ItemHandler::addGroundItem(unsigned short itemId, unsigned short x, unsigned short y, unsigned short quantity)
{
    int index = -1;
    std::shared_ptr<ITEM_DROP> item_drop;

    /* Find a free slot if any */
    for (unsigned int i = 0; i < item_drops.size(); i++)
    {
        if (item_drops[i] == NULL)
        {
            index = i;
        }
    }

    if (index == -1)
    {
        item_drop = std::make_shared<ITEM_DROP>();
        item_drops.push_back(item_drop);
    }
    else
    {
        item_drop = item_drops[index];
    }

    item_drop->itemId = itemId;
    item_drop->x = x;
    item_drop->y = y;
    item_drop->quantity = quantity;
}

[helios]

Which part specifically?

[danielmccarthy]

Well I check to see if their is a null pointer in the vector if not I extend the vector. Is their a better way of checking and doing this or is my code above alright?

[helios]

You could use a linked list of items.

[danielmccarthy]

Any example?

Also when I null one of the item_drops I get a segmentation fault when a new ground item is added. The crash is at line 25 of the code above

Any idea why?

Here is the delete code:

void ItemHandler::process()
{
    for (unsigned int i = 0; i < item_drops.size(); i++)
    {
        auto itemDrop = item_drops[i];
        if (itemDrop != NULL)
        {
            /* Delete the item after it has been registered for 10 seconds */
            if ((Misc::getTimeSeconds() - itemDrop->registerTime_Seconds) > 10)
            {
                cout << "Deleteing item: " << itemDrop->itemId << endl;
                item_drops[i] = 0;
            }
        }
    }
}

[danielmccarthy]

Ah I found the bug for that problem I fixed the code

void ItemHandler::addGroundItem(unsigned short itemId, unsigned short x, unsigned short y, unsigned short quantity)
{
    int index = -1;
    std::shared_ptr<ITEM_DROP> item_drop = std::make_shared<ITEM_DROP>();

    /* Find a free slot if any */
    for (unsigned int i = 0; i < item_drops.size(); i++)
    {
        if (item_drops[i] == NULL)
        {
            index = i;
            break;
        }
    }

    if (index == -1)
    {
        item_drops.push_back(item_drop);
    }
    else
    {
        item_drops[index] = item_drop;
    }

    item_drop->itemId = itemId;
    item_drop->x = x;
    item_drop->y = y;
    item_drop->quantity = quantity;
    item_drop->registerTime_Seconds = Misc::getTimeSeconds();

    /* Send all players the floor item */
    for (int i = 0; i < PlayerHandler::maxPlayers; i++)
    {
        Client* c = (Client*) PlayerHandler::player[i];
        if (c != NULL)
        {
            c->addGroundItem(itemId, x, y);
        }
    }
}

[dhayden]

It looks like item_drops is a vector. The way you're using it here, a list would be better. That way you wouldn't have to check for an empty slot.

Do you really need shared pointers here? If item_drops owns the items then you could save time and space with unique_ptr instead.

If you don't need a vector and you don't really need shared pointers then you'd be much better of with a list<ITEM_DROP>. No pointers at all.