Remove elements of a vector inside the loop

Question

I know that there are similar questions to this one, but I didn’t manage to find the way on my code by their aid. I want merely to delete/remove an element of a vector by checking an attribute of this element inside a loop. How can I do that? I tried the following code but I receive the vague message of error:

'operator =' function is unavailable in 'Player’.

 for (vector<Player>::iterator it = allPlayers.begin(); it != allPlayers.end(); it++)
 {
     if(it->getpMoney()<=0) 
         it = allPlayers.erase(it);
     else 
         ++it;
 }

What should I do?

Update: Do you think that the question vector::erase with pointer member pertains to the same problem? Do I need hence an assignment operator? Why?

Whatsoever

Please note that that you could be a lot better off using std::remove_if. Please see this post for details on that. – Dreary

Use the erase/remove idiom as described in this post. – Enwind

Answer 1

You should not increment it in the for loop:

for (vector<Player>::iterator it=allPlayers.begin(); 
                              it!=allPlayers.end(); 
                              /*it++*/) <----------- I commented it.
{

   if(it->getpMoney()<=0) 
      it = allPlayers.erase(it);
  else 
      ++it;
 }

Notice the commented part;it++ is not needed there, as it is getting incremented in the for-body itself.

As for the error "'operator =' function is unavailable in 'Player’", it comes from the usage of erase() which internally uses operator= to move elements in the vector. In order to use erase(), the objects of class Player must be assignable, which means you need to implement operator= for Player class.

Anyway, you should avoid raw loop¹ as much as possible and should prefer to use algorithms instead. In this case, the popular Erase-Remove Idiom can simplify what you're doing.

allPlayers.erase(
    std::remove_if(
        allPlayers.begin(), 
        allPlayers.end(),
        [](Player const & p) { return p.getpMoney() <= 0; }
    ), 
    allPlayers.end()
); 

^{1. It's one of the best talks by Sean Parent that I've ever watched.}

Prau

I tried this but I receive the same error. When I remove the above loop (of deleting) the programs compiles. Consequently, there is a problem with the delete/erase. There are members of class Player that are pointers to other objects. What become they in this case? – Whatsoever

Actually the error comes from std::vector.erase, which uses the assignment operator to move elements in order to keep the vector contiguous. – Mongo

does this idiom have a name? – Sanity

This is a horrible answer! Iterators are invalidated after erasing an element!!!! – Scintilla

@TheQuantumPhysicist: Yes, that is true, which is why I did this : it = allPlayers.erase(it); please see the assignment carefully! Or else feel free to post better answer. – Prau

Thanks for clarifying. I see now why it's valid. Though I prefer the other answer with decreasing numbers. – Scintilla

@TheQuantumPhysicist: which other answers? all of them are doing the same thing, just that they've used library which hides these details. But then if you dont know the details, you'll more likely to say "horrible answer" when you're exposed to their internals as to how they work! – Prau

OK. Try again and try to find an answer that is: 1- Other than yours, 2- Doesn't use any external library, 3- has a decreasing loop. Once you find it, you'll know why I prefer it over yours (probably) :-) . Remember, it's a taste thing, and I already acknowledged that your answer is OK. For my taste, personally, I avoid using iterators with all containers that support operator[], unless there's a performance issue there. – Scintilla

@TheQuantumPhysicist: Does any of other answers explain why the OP's solution does not work? A good answer is supposed to explain it as well, right? Or can you please post an answer, explaining why OP's attempt doesn't work and then fix the bug? I'm really interested to see how your answer is going to differ from mine. – Prau

That's not really my concern. Have a nice day! – Scintilla

@TheQuantumPhysicist: If you say an answer is not good compared to others, then I think you have to explain why is that? and do them explain the issue in the solution attempted in the question itself? In my opinion, none of the posted answers currently explains the issue. They just have proposed another solution, without explaining why the attempted solution doesn't work. – Prau

My concern as someone who found this with Google is a reliable answer that works and fits my perspective. I explained already why I like the other answer more. – Scintilla

@TheQuantumPhysicist: those are not answers to the question as they do not address the issue in the question. – Prau

Answer 2

if(allPlayers.empty() == false) {
    for(int i = allPlayers.size() - 1; i >= 0; i--) {
        if(allPlayers.at(i).getpMoney() <= 0) {
            allPlayers.erase( allPlayers.begin() + i ); 
        }
    }
}

This is my way to remove elements in vector. It's easy to understand and doesn't need any tricks.

Debbee

Just a quick comment: you can replace (allPlayers.empty() == false) by just saying (!allPlayers.empty()). This is because empty() returns a boolean type: if the vector is empty it will return true. Using the "not" operator is like saying "if it's NOT true that the vector is empty". Just to prettify your code :) – Softy

@Anarelle Thanks! – Debbee

This reminds me that I should NOT erase from the beginning (i == 0). Because each time erase() is called, then begin() will be changed at the same time. begin() + i will be changed according to new vector (one item was just erased). If erase from the end to beginning, it will be OK. Thanks:) – Goss

This will yield valid result, but is inefficient as can be, as subsequent elements will be moved repeatedly towards front for each element removed. – Crimmer

Answer 3

Forget the loop and use the std or boost range algorthims.
Using Boost.Range en Lambda it would look like this:

boost::remove_if( allPlayers, bind(&Player::getpMoney, _1)<=0 );

Batavia

+1. This is the way to go! – Dreary

-1 for a disingenuous answer. For example, how would one write said algorithms without knowing how to do it at a lower level. Not everyone can live in Abstraction Heaven. About as useful as someone answering USE JQUERY!!1! for someone trying to learn Javascript. – Lauricelaurie

This algorithm is useful only when you just want to delete the elements. Think about scenario, if(condition) it = x.erase(it); else { file << *it; ++it; }. As you can see if you want to do something else when the element is not fit for deleting, you cannot use remove_if. Even if you use it, you may have to traverse through the loop again. – Oldcastle

Answer 4

Your specific problem is that your Player class does not have an assignment operator. You must make "Player" either copyable or movable in order to remove it from a vector. This is due to that vector needs to be contiguous and therefore needs to reorder elements in order to fill gaps created when you remove elements.

Also:

Use std algorithm

allPlayers.erase(std::remove_if(allPlayers.begin(), allPlayers.end(), [](const Player& player)
{
    return player.getpMoney() <= 0;
}), allPlayers.end());

or even simpler if you have boost:

boost::remove_erase_if(allPlayers, [](const Player& player)
{
    return player.getpMoney() <= 0;
});

See TimW's answer if you don't have support for C++11 lambdas.

Mongo

I think also that the problem is what you mention. However, I have added an assignement operator as Player& operator= (const Player& rhs); in the Player.h file but I still get error(with different message). Do I need finallly a copy constructor? – Whatsoever

You should also implement a copy constructor. It is difficult to tell what the problem is, if you don't post the relevant error nor code. – Mongo

Answer 5

Or do the loop backwards.

for (vector<Player>::iterator it = allPlayers.end() - 1; it != allPlayers.begin() - 1; it--)
    if(it->getpMoney()<=0) 
        it = allPlayers.erase(it);

Contradance

Answer 6

C++11 has introduced a new collection of functions that will be of use here.

allPlayers.erase(
    std::remove_if(allPlayers.begin(), allPlayers.end(),
        [](auto& x) {return x->getpMoney() <= 0;} ), 
    allPlayers.end()); 

And then you get the advantage of not having to do quite so much shifting of end elements.

Riggle

std::vector::erase(iterator) removes a single element pointed to by the iterator. In your example, it will attempt to remove the element pointed to by the iterator returned by std::remove_if -- which is a pass-the-end iterator so this is almost certainly incorrect (and will cause a crash). It should be: allPlayers.erase(std::remove_if(...), allPlayers.end()) which instead removes all elements in a range. – Blaseio

Answer 7

A modern C++20 way to delete specific elements from a std::vector is as follows:

std::vector vector = { 1, 2, 3, 4, 5, 6, 7, 8 };
std::erase_if(vector, [](int const& i) { return i % 2 == 0; });

Now all even elements are removed from the std::vector. No issues with index shifts while iterating or whatever.

Gorilla

Answer 8

Late answer, but as having seen inefficient variants:

1. std::remove or std::remove_if is the way to go.
2. If for any reason those are not available or cannot be used for whatever other reason, do what these hide away from you.

Code for removing elements efficiently:

auto pos = container.begin();
for(auto i = container.begin(); i != container.end(); ++i)
{
    if(isKeepElement(*i)) // whatever condition...
    {
        if(i != pos)
        {
            *pos = *i; // will move, if move assignment is available...
        }
        ++pos;
    }
}
// well, std::remove(_if) stops here...
container.erase(pos, container.end());

You might need to write such a loop explicitly e. g. if you need the iterator itself to determine if the element is to be removed (the condition parameter needs to accept a reference to element, remember?), e. g. due to specific relationship to successor/predecessor (if this relationship is equality, though, there is std::unique).

Crimmer