CODEWITHSUNDEEP
c++vectorerase
arjacsoh
arjacsoh

Reputation: 9232

Remove elements of a vector inside the loop

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?

Upvotes: 88

Views: 140111

Answers (8)

BullyWiiPlaza
BullyWiiPlaza

Reputation: 19195

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.

Upvotes: 2

Aconcagua
Aconcagua

Reputation: 25526

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

Upvotes: 0

Sarfaraz Nawaz
Sarfaraz Nawaz

Reputation: 361482

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 loop1 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.

Upvotes: 178

UKMonkey
UKMonkey

Reputation: 6983

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.

Upvotes: 3

hhhhhhhhh
hhhhhhhhh

Reputation: 113

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

Upvotes: 4

Dawoon Yi
Dawoon Yi

Reputation: 436

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.

Upvotes: 21

ronag
ronag

Reputation: 51253

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.

Upvotes: 5

TimW
TimW

Reputation: 8447

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

Upvotes: 9

Related Questions