86

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?

2
  • 2
    Please note that that you could be a lot better off using std::remove_if. Please see this post for details on that.
    – user405725
    Jan 14, 2013 at 15:12
  • Use the erase/remove idiom as described in this post.
    – Mukesh MV
    Apr 1, 2017 at 14:33

8 Answers 8

175

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.

13
  • 1
    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?
    – arjacsoh
    Dec 25, 2011 at 9:43
  • 1
    Actually the error comes from std::vector.erase, which uses the assignment operator to move elements in order to keep the vector contiguous.
    – ronag
    Dec 25, 2011 at 11:52
  • does this idiom have a name?
    – sp2danny
    May 16, 2015 at 12:36
  • 1
    This is a horrible answer! Iterators are invalidated after erasing an element!!!! Aug 4, 2016 at 16:28
  • 10
    @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. Aug 4, 2016 at 18:03
21
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.

4
  • 3
    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 :)
    – Floella
    Oct 17, 2016 at 13:04
  • @Anarelle Thanks!
    – Dawoon Yi
    Nov 3, 2016 at 6:47
  • 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:)
    – galian
    Dec 12, 2017 at 16:07
  • 1
    This will yield valid result, but is inefficient as can be, as subsequent elements will be moved repeatedly towards front for each element removed.
    – Aconcagua
    Sep 14, 2018 at 2:00
9

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 );
3
  • 3
    +1. This is the way to go!
    – user405725
    Jan 14, 2013 at 15:13
  • 40
    -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. Sep 26, 2013 at 17:53
  • 3
    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.
    – iammilind
    Jul 13, 2015 at 10:06
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.

2
  • 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?
    – arjacsoh
    Dec 25, 2011 at 11:52
  • 1
    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.
    – ronag
    Dec 25, 2011 at 11:54
4

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

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.

1
  • 2
    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.
    – Ellis
    Mar 7, 2018 at 19:42
2

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.

0

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.