C++ Deleting an object in a vector

Question

I am currently using a vector to hold the people in a program. I am trying to delete it with

vectorname.erase(index);

I pass the vector in a function, as well as the element that I want to delete. My main problem is how can I improve my code in terms of compilation speed?

#include <iostream>
#include <string>
#include <vector>
using namespace std;

class person {
    private:
        string name;
    public:
        person() {}
        person(string n):name(n){}
        const string GetName() {return name;}
        void SetName(string a) { name = a; }
};

void DeleteFromVector(vector<person>& listOfPeople,person target) {
    for (vector<person>::iterator it = listOfPeople.begin();it != listOfPeople.end();++it) {//Error 2-4
        if (it->GetName() == target.GetName()) {
            listOfPeople.erase(it);
            break;
        }
    }
}

int main(){
    //first group of people
    person player("Player"), assistant("Assistant"), janitor("Janitor"), old_professor("Old Professor");

    //init of vector
    vector<person> listOfPeople = { player, assistant, janitor, old_professor };

    DeleteFromVector(listOfPeople, janitor);
}

You're doing this all wrong. You're using `erase` but not taking its return value as the new iterator, so after that you're using invalid, corrupt iterators. And, there's a much better way to do it which you can see here http://en.cppreference.com/w/cpp/algorithm/remove and https://en.wikipedia.org/wiki/Erase%E2%80%93remove_idiom — Zan Lynx, Mar 27 '17 at 01:53
OK, I see your `break` immediately after using `erase` so I guess you're not using invalid iterators after all. — Zan Lynx, Mar 27 '17 at 01:55
Instead of all that stuff, just use `find_if` to find the janitor and erase using it's return value. [Example](http://coliru.stacked-crooked.com/a/f004aab3525a942a). — Paul Rooney, Mar 27 '17 at 02:13
Why do you use both `it` and `index`. The whole point of an iterator is to use it instead of an index. Pick one or the other but not both. — M.M, Mar 27 '17 at 02:52

zhm · Accepted Answer · 2017-03-27T02:22:09.233

7

There is no need to define index, iterator can be used to access objects in vector:

for (vector<person>::iterator it = listOfPeople.begin(); it != listOfPeople.end(); ++it) {//Error 2-4
    if (it->GetName() == target.GetName()) {
        listOfPeople.erase(it);
        break;
    }
}

Since next line is to break for loop, we don't consider invalid iterator problem here.

edited Mar 27 '17 at 02:22

answered Mar 27 '17 at 01:51

zhm

3,513
3
34
55

1

Next line is to break for loop, should we still need to consider invalid iterator problem? – zhm Mar 27 '17 at 01:55
Ah, that would avoid the problem. – Bo Persson Mar 27 '17 at 01:57
@MartinZhai Thanks for the info :) – CraftedGaming Mar 27 '17 at 02:34

score 3 · Answer 2 · answered Mar 27 '17 at 04:38

3

You don't need that loop to delete the object from the vector. Just use std::find_if:

#include <algorithm>
//...
void DeleteFromVector(vector<person>& listOfPeople, const person& target) 
{
    // find the element
    auto iter = std::find_if(listOfPeople.begin(), listOfPeople.end(),
                             [&](const person& p){return p.GetName() == target.GetName();});

    // if found, erase it
    if ( iter != listOfPeople.end())
       listOfPeople.erase(iter);
}

answered Mar 27 '17 at 04:38

PaulMcKenzie

34,698
4
24
45

This one is better than the accepted answer - use the std::find_if instead of doing the work yourself. And if there is operator == defined for the person class you can use std::find() – Tim Mar 27 '17 at 05:05
@PaulMcKenzie, the "p" object in the return line and the "target" is causing errors. Error (active) the object has type qualifiers that are not compatible with the member function "person::GetName" Error (active) the object has type qualifiers that are not compatible with the member function "person::GetName" Error C2662 'std::string person::GetName(void)': cannot convert 'this' pointer from 'const person' to 'person &' – CraftedGaming Mar 27 '17 at 06:10
@CraftedGaming Then either remove the `const` or make `GetName()` a const function. Better to make it a `const` function. – PaulMcKenzie Mar 27 '17 at 15:01
@PaulMcKenzie I changed the function into a const however it still contains the same set of errors – CraftedGaming Mar 28 '17 at 10:13
@CraftedGaming -- It compiles with no errors [here](http://coliru.stacked-crooked.com/a/60da3d99c6b0472f) – PaulMcKenzie Mar 28 '17 at 15:12
@PaulMcKenzie Screw it. It's not compiling on my side. I'm still going to use the first answer – CraftedGaming Mar 29 '17 at 05:55
@CraftedGaming -- Maybe you're not using a C++ 11 compiler? What compiler are you using? It also compiles with no errors [using Visual Studio 2015](http://rextester.com/BIOB98935). So if you're not getting it to compile, you need to look into why, as your compiler may be broken. – PaulMcKenzie Mar 29 '17 at 12:26

djacob · Answer 3 · 2020-05-29T02:09:45.750

1

listOfPeople.erase(
                   remove(listOfPeople(), listOfPeople.end(), target),
                   listOfPeople.end()
                  )

"remove" operation in this erase-remove idiom will move all the elements except target to the front of the vector range and "erase" operation will delete all elements that are at the end that satisfies the target criteria. This is much efficient even though it is not as expressive as iterative version.

edited May 29 '20 at 02:09

answered May 27 '20 at 14:04

djacob

110
1
9

Hope It will solve issue but please add explanation of your code with it so user will get perfect understanding which he/she really wants. – Jaimil Patel May 27 '20 at 14:24
See "[Explaining entirely code-based answers](https://meta.stackoverflow.com/q/392712/128421)". While this might be technically correct it doesn't explain why it solves the problem or should be the selected answer. We should educate in addition to help solve the problem. – the Tin Man May 27 '20 at 21:28
Thanks both. answer is now updated with explanation. – djacob May 29 '20 at 02:16

C++ Deleting an object in a vector

3 Answers3