Iterate Vector, Remove Certain Items as I Go

iterate vector, remove certain items as I go

Check out std::remove_if:

#include <algorithm> // for remove_if
#include <functional> // for unary_function

struct delete_file : public std::unary_function<const std::string&, bool> 
{
    bool operator()(const std::string& strPath) const
    {
        return ::DeleteFile(strPath.c_str());
    }
}

m_vPaths.erase(std::remove_if(m_vPaths.begin(), m_vPaths.end(), delete_file()),
                m_vPaths.end());

Use a std::list to stop the invalid iterators problem, though you lose random access. (And cache performance, in general)

---

For the record, the way you would implement your code would be:

typedef std::vector<std::string> string_vector;
typedef std::vector<std::string>::iterator string_vector_iterator;

string_vector_iterator iter = m_vPaths.begin();
while (iter != m_vPaths.end())
{
    if(::DeleteFile(iter->c_str()))
    {
        // erase returns the new iterator
        iter = m_vPaths.erase(iter);
    }
    else
    {
        ++iter;
    }
}

But you should use std::remove_if (reinventing the wheel is bad).

Remove elements of a vector inside the loop

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

Vector erase iterator

res.erase(it) always returns the next valid iterator, if you erase the last element it will point to .end()

At the end of the loop ++it is always called, so you increment .end() which is not allowed.

Simply checking for .end() still leaves a bug though, as you always skip an element on every iteration (it gets 'incremented' by the return from .erase(), and then again by the loop)

You probably want something like:

 while (it != res.end()) {
        it = res.erase(it);    
 }

to erase each element

(for completeness: I assume this is a simplified example, if you simply want every element gone without having to perform an operation on it (e.g. delete) you should simply call res.clear())

When you only conditionally erase elements, you probably want something like

for ( ; it != res.end(); ) {
  if (condition) {
    it = res.erase(it);
  } else {
    ++it;
  }
}

Erase some of a vector's elements in a for-each loop without iterating the whole vector

You should still use std::remove_if, just call std::find beforehand.

auto el = std::find(vec.begin(), vec.end(), whatImLookingFor);
auto p = std::remove_if(vec.begin(), el, isInvalid);

// returns the iterator, not the element itself.
// if the element is not found, el will be vec.end()
return vec.erase(p, el);

This will usually be more efficient than removing one element at a time.

Removing item from vector, while in C++11 range 'for' loop?

No, you can't. Range-based for is for when you need to access each element of a container once.

You should use the normal for loop or one of its cousins if you need to modify the container as you go along, access an element more than once, or otherwise iterate in a non-linear fashion through the container.

For example:

auto i = std::begin(inv);

while (i != std::end(inv)) {
    // Do some stuff
    if (blah)
        i = inv.erase(i);
    else
        ++i;
}

---

Related Topics