0

So I want to compare the 0th element of a vector with the other elements to see if they're equal because I want to remove other instances of that element's value from the vector e.g. {1, 1, 2, 3, 1} becomes {1, 2, 3} and this is the code I wrote:

std::vector<int> arr = {1,1,5,5,1,1};
for (int k = 1; k < arr.size(); k++)
{
    if(arr[0] == arr[k]) {
    arr.erase(arr.begin() + k);
}

The output I expected from this was:

155

Since it's supposed to remove all instances of the 1 except the first one, but what I instead get is:

1551

Where'd the last 1 come from and how do I fix this?

Vlad from Moscow
  • 301,070
  • 26
  • 186
  • 335
NotApollo
  • 39
  • 3
  • I wouldn't expect any output at all from this. Please take the habit of extracting a [mcve] first and include that in your question. That said, if you step through this with a debugger (learn that, it's invaluable!) or output the operations you do and their parameters using `std::cout`, you should quickly find out what you did wrong. – Ulrich Eckhardt Aug 15 '20 at 08:12
  • You want to use `std::remove_if` in conjunction with `vector::erase`. – n. m. could be an AI Aug 15 '20 at 08:22

2 Answers2

3

The loop is written incorrectly.

for (int k = 1; k < arr.size(); k++)
{
    if(arr[0] == arr[k]) {
    arr.erase(arr.begin() + k);
}

You should write at least

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

But in any case it is better to use the standard algorithm std::remove. For example

arr.erase( std::remove( std::next( std::begin( arr ) ), std::end( arr ), arr[0] ), std::end( arr ) );

Here is a demonstrative program.

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

int main() 
{
    std::vector<int> arr = { 1, 1, 5, 5, 1, 1 };

    arr.erase( std::remove( std::next( std::begin( arr ) ), std::end( arr ), arr[0] ),
               std::end( arr ) );
               
    for ( const auto &item : arr ) std::cout << item << ' ';
    std::cout << '\n';
    
    return 0;
}

Its output is

1 5 5
Vlad from Moscow
  • 301,070
  • 26
  • 186
  • 335
  • Thank you! You're first code worked but I never tried the second one because I have a hard time reading it since I'm new. Would you mind explaining it more? – NotApollo Aug 15 '20 at 08:23
  • @NotApollo In the loop you are deleting each element separately. This is inefficient because after deleting each element other elements are moved to the position of the deleted elements. The algorithm std::remove at first moves all deleted elements to the end of the vector and the member function erase remove them all at once. – Vlad from Moscow Aug 15 '20 at 08:28
2

The issue is that the index of your for-loop is keeping increasing after you removed an element. Just imagine that k is 1. You remove the arr[1] element. After that arr[1] is 5 but the index is increasing to 2. So actually you need to check arr[1] again but you won't. So in this way you skip some elements in the array.

I propose to replace for loop by while loop and increase the index only if you don't erase an element.

rhaport
  • 540
  • 3
  • 11