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?

Answer 1

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.

Answer 2

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 talk by Sean Parent that I've ever watched.}

Answer 3

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);

Answer 4

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.

Answer 5

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.

Answer 6

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...
    {
        *pos++ = *i; // will move, if move assignment is available...
    }
}
// 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).