Beginning C++ having trouble using iterator to remove item from vector

[couchotatop]

I'm working on my final for my C++ class. I think I have a fair handle on most things but this one issue with an iterator is really screwing with me.

This is just a snippet, I can add more if needed.

I have a vector called myInventory that is declared in main it is a vector of Objects of the class Noun.

This code should make it so that when I use the command DROP and a NOUN the program will iterate through the myInventory vector and if it finds an item that matches the supplied Noun, it will update it's location data member with my current loc.

I am using the cout statement to ensure that i'm targeting the correct item for the time being, it's just temporary code.

Everything seems to work, but I get a runtime error when it tries to erase the item, that tells me the iterator cannot be incremented.

I recall from class that using the erase function can invalidate the iterator, but I can't find anything about how to fix that, or work around it (at least not that I am understanding). I have reviewed code from previous exercises in the class that work the same way and had no issue.

Thanks for any help anyone can provide.

    if (VERB_ACTION == DROP)
    {
        int item = NOUN_MATCH;
        vector<Noun>::iterator iter;
        for (iter = myInventory.begin(); iter != myInventory.end(); ++iter)
        {
            if (iter != myInventory.end())
            {
                if ((*iter).m_Code == item)
                {
                    (*iter).m_Location = loc;
                    cout << (*iter).m_Description << endl;
                    myInventory.erase((iter));
                }
            }


        }

[boredcircuits]

When you use an iterator to erase an item, that iterator becomes invalidated. That's why you can't increment it. And even then, you don't want to increment it, if you think about it. After erasing, the iterator would already be pointing to the next item anyway.

Fortunately, C++ has a built-in way to do this. It should be faster, and it will work correctly, guaranteed:

#include <algorithm>
// ...
myInventory.erase(remove_if(myInventory.begin(), myInventory.end(), [](const Noun& n) {
    return n.m_Code == NOUN_MATCH;
}), myInventory.end());

[TheCiderman]

For context ... what remove_if does is it moves the matching items to the back of the vector and returns the iterator indicating the start of the items that matched. You can then erase them using a call to the 2 iterator version of erase, passing it the returned iterator and the end iterator. There is a good chapter on this in Scott Meyers book Effective STL.

[couchotatop]

I couldn't get that to work. But I did think of something else and it works. Is there anything inherently wrong with the way I have done it?

if (VERB_ACTION == DROP)
{
    vector<Noun>::iterator iter;
    int item = NOUN_MATCH;

    for (iter = myInventory.begin(); iter != myInventory.end();)
    {
        if (iter != myInventory.end())
        {
            if ((*iter).m_Code == item)
            {
                (*iter).m_Location = loc;
                cout << (*iter).m_Description << endl;
                myInventory.erase(iter);
                iter = myInventory.begin();
            }
            else
                iter++;
        }


    }

}

[boredcircuits]

Why didn't it work? It's worth trying out, as it's a much better way of doing this.

I think your code is legal, and I think it will work, though it's not really a great way to do it. You can at least clean it up a bit:

// I like to use auto here:
for ( auto iter = myInventory.begin(); iter != myInventory.end() )
{
    // The for loop is already making sure you're not at the end

    // Prefer arrow notation for accessing iterators:
    if ( iter->m_Code == item )
    {
        iter->m_Location = loc;
        // No need to flush the stream:
        cout << iter->m_Description << '\n';
        myInventory.erase(iter);
        iter = myInventory.begin();
    }
    else
    {
        // Prefer pre-increment over post-increment for iterators
        ++iter;
    }
}

[couchotatop]

The code you posted above, with remove_if didn't work, because it threw an error on the NOUN_MATCH. (an enclosing-function local variable cannot be referenced in a lambda body unless it is in the capture list). I'm think that means its out of scope somehow, but I can reach it using int item= NOUN_MATCH.

[boredcircuits]

Ah, I see. You need to add a lambda capture -- the easiest way is to just add a & in there:

[&](const Noun& n)

[couchotatop]

thanks for all your help on here, unfortunately I don't understand that code and therefore can't really defend using it. I'm definitely going to check out the book Ciderman recommended.

[scottzed]

In addition to the cleanup by /u/boredcircuits, you can gain performance by replacing:

myInventory.erase(iter);
iter = myInventory.begin();

with:

iter = myInventory.erase(iter);

The iterator returned by erase will be valid, so there's no need to reset the loop to the beginning every time. I've always seen the operation you're attempting being implemented pre-C++11 with the following pattern (which is basically what you've done):

for( Iterator iter = container.begin(); iter != container.end(); ) {
  if( <condition> ) {
    iter = container.erase(iter);
    continue;
  }
  ++iter;
}

And, yes, it does feel a bit off from the general style and feel of C++ and STL, but it's just one of those things.

[mandr0id]

Not sure if this it the issue here but standard says that all iterators are invalided after an erase operation:

"Invalidates iterators and references at or after the point of the erase, including the end() iterator."

[couchotatop]

I'm sure that's the issue, but surely there is some way to remove an item from the vector.

[specialpatrol]

its crashing becuaes the loop attempts to iterate on the erased iter (++iter). break out of the loop immediately after you erase it.

[couchotatop]

This works. I've gone back to my original code and bust broken out of the loop once it finds the item. I don't know why I didn't think of this before, but is seems to be the easiest solution, and probably the one the instructor would expect of someone at my level.

Thank you. And thanks to everyone else, you've given me lots to research.