2

Can someone please explain me why this code doesn't erase all 1s from the vector:

for (int i = 0; i < numbers.size(); i++)
{
    if (numbers[i] == 1)
    {
        numbers.erase(numbers.begin() + i);
    }
}
user3650284
  • 137
  • 1
  • 3
  • 10
  • 2
    Use `std::remove`. Also see the erase-remove idiom and why it exists. – chris Aug 23 '14 at 12:47
  • Ok, so I used erase-remove idiom as suggested by chris and here is the code I got: `numbers.erase(remove(numbers.begin(), numbers.end(), i), numbers.end());`, I get error from IntelliSense: `Error 2 error C2660: 'remove' : function does not take 3 arguments`. What am I doing wrong? – user3650284 Aug 23 '14 at 12:55
  • @user3650284, You didn't include `` – chris Aug 23 '14 at 12:56
  • @chris Ok, I included algorithm, now 90% of my vector elements are 1, they are not removed. – user3650284 Aug 23 '14 at 12:59
  • 1
    Sure you wanted to use `i`? It should just replace your loop and use 1. – chris Aug 23 '14 at 13:02
  • @chris That's it. I replaced i with 1 in erase-remove idiom and now my code works! I'm facing with this for the first time. Thank you for the answer. – user3650284 Aug 23 '14 at 13:05

3 Answers3

6

Let's try it out.

Say you have a vector with the contents [1, 1, 1] (all ones, just to keep it simple)

First iteration:

for (int i = 0; i < numbers.size(); i++) <-- i == 0; numbers.size() == 3
{
    if (numbers[i] == 1) <-- true
    {
        numbers.erase(numbers.begin() + i); <-- erase is called on element #0
    }
}

Second iteration: The vector now contains [1, 1] because we removed the 0th entry.

for (int i = 0; i < numbers.size(); i++) <-- i == 1; numbers.size() == 2
{
    if (numbers[i] == 1) <-- true
    {
        numbers.erase(numbers.begin() + i); <-- erase is called on element #1
    }
}

Third iteration: The vector now contains [1]

for (int i = 0; i < numbers.size(); i++) <-- i == 2; numbers.size() == 1; the loop condition is false, so we exit the loop
{
    if (numbers[i] == 1)
    {
        numbers.erase(numbers.begin() + i);
    }
}

Final result:

the vector contains [1].

As you can probably see from evaluating the code line by line manually, the problem is that you increment i even after removing an element. Every time you remove an element, you shift all the remaining elment to lower indices, but at the same time, you increment the counter so the next index you look at is one higher. So when you remove element i, the element that was previously index i+1 is moved to index i. But in the next iteration, you no longer look at index i, but instead i+1, so you skip past an element without ever looking at it.

jalf
  • 243,077
  • 51
  • 345
  • 550
  • Thank you for this analysis, this makes things more clear to me. Is it possible to somehow use `for (int x : numbers)` to delete x element? – user3650284 Aug 23 '14 at 13:03
  • The conventional approach is the "erase/remove idiom". The idea is to use `std::remove` on the iterator range `numbers.begin(), numbers.end()` to remove all the elements you want to get rid of. This returns a new iterator pointing to the end of the "non-removed" sequence. Then you pass that iterator and `numbers.end()` to `numbers.erase` to erase from the vector everything between the end of the cleaned-up sequence and the end of the vector – jalf Aug 23 '14 at 13:11
3

Your code says i < numbers.size(), and you increment i and also delete n[i](if it has 1) After deleting , you should not increment i because a new number gets in the currrent position of i.

If numbers[]={0,1,2,3,1,4} -> you get numbers[]={0,2,3,4}

If numbers[]={0,1,1,2,3,1,4} -> you get numbers[]={0,1,2,3,4},

i.e after deleting , it skips the next number just add i--; after deleting numbers[i]

for (int i = 0; i < numbers.size(); i++){ if (numbers[i] == 1){ numbers.erase(numbers.begin() + i); i--; } } This will give you the correct answer.

Shankar
  • 417
  • 1
  • 6
  • 17
1

When an element of a vector is erased then all elements after the erased element are moved left. So for example if the element with index 0 was erased then the first actual element of the vector will have index equal to 9. You must not to increase the index in this case.

The valid loop can look like

for ( int i = 0; i < numbers.size(); )
{
    if ( numbers[i] == 1 )
    {
        numbers.erase( numbers.begin() + i );
    }
    else
    {
        ++i;
    }
}

Take into account that it is simpler to apply standard algorithm std::remove declared in header <algorithm>

For example

#include <algorithm>

//...

numbers.erase( std::remove( numbers.begin(), numbers.end(), 1 ), numbers.end() );

Here is a demonstrative example

#include <iostream>
#include <vector>
#include <algorithm>

int main() 
{
    std::vector<int> v = { 1, 1, 2, 1, 3, 1, 4, 1, 5 };

    for ( int x : v ) std::cout << x << ' ';
    std::cout << std::endl;

    v.erase( std::remove( v.begin(), v.end(), 1 ), v.end() );

    for ( int x : v ) std::cout << x << ' ';
    std::cout << std::endl;

    return 0;
}

The output is

1 1 2 1 3 1 4 1 5 
2 3 4 5
Vlad from Moscow
  • 301,070
  • 26
  • 186
  • 335