Delete an object from a vector of objects

Question

I have a vector of objects.. monsters. and i have a function "shoot". i want that when i kill one of the monsters the object to be deleted. i've tryed various ways but no succes. Here's how i've created the objects:

void AdaugareMonstru(vector<monstru>& Nmonstru){
    int i = 0;
        srand(time(0));
        int a, b;
        for (i; i < 4; i++){
            a = (rand() % 5)+1;
            b = (rand() % 8)+1;

            while (a == 1 && b == 1){
                a = (rand() % 5)+1;
                b = (rand() % 8)+1;
            }
            monstru newMonstru(a,b);
            Nmonstru.push_back(newMonstru);
        }
    }

And here is the function where i find the monster in front of my "hero" and i kill it... or not..cuz i don't know how to delete it :D I think there is a problem with my declaration of the "trage " function but i can't figure it out.

    void trage(vector<monstru>& monstru, vector< vector<char> > map,int x,int y,int face){
        if (face == 1)
        {
            for (int i = y; i < 10; i++){
                if (monstru[0].GetX() == x && monstru[0].GetY() == i){
                    monstru[0].SetX(0);
                    monstru[0].SetY(0);
                    cout << "Ai ucis un monstru" << endl;
                }
                if (monstru[1].GetX() == x && monstru[0].GetY() == i){
                    monstru[1].SetX(0);
                    monstru[1].SetY(0);
                    cout << "Ai ucis un monstru" << endl;
                }
                if (monstru[2].GetX() == x && monstru[0].GetY() == i){
                    monstru[2].SetX(0);
                    monstru[2].SetY(0);
                    cout << "Ai ucis un monstru" << endl;
                }
                if (monstru[3].GetX() == x && monstru[0].GetY() == i){
                    monstru[3].SetX(0);
                    monstru[3].SetY(0);
// Instead of setting X and Y to 0 i want to delete the whole object.
                    monstru.erase(1); // doesn't work... no wonder:D
                    delete monstru[0]; // doesn't work either.
                    cout << "Ai ucis un monstru" << endl;
                }
            }
        }
    }

First never use delete unless you have used new. Secondly you need an iterator to the monster you want to erase and it is that iterator that you pass to `erase()` — NathanOliver, Aug 28 '15 at 19:04
monstru.erase(monstru.begin() + i); should work, but you should look into better ways to find which monster is in front of the character. — user4581301, Aug 28 '15 at 19:06
Looks like this works.I couldn't think of a better way to find what's in front of my character even that what i used is the worse... ill try more. thanks :D — Teo Alex, Aug 28 '15 at 19:11

score 2 · Answer 1 · edited May 23 '17 at 11:58

erase removes an object from the vector and destroys it. If you still need the object, get a copy of it first. If you have pointers to it, treat them like toxic waste. Don't touch them and get rid of them as fast as possible. Come to think of it, don't take pointers to vector elements in the first place. Any insert or erase operation will leave them either pointing to the wrong element or completely invalid. If you have outstanding iterators for the vector, consider them invalid.

In the OP's code, monstru.erase(monstru.begin() + i); will slay the monster.

But there are better ways of finding the monster to slay. Malte Schmitz has demonstrated the brute force approach. The following is a little less brutish.

First add a convenience function to class monstru

bool isAt(int x, int y) const
{
    return (mX == x && mY == y);
}

Where mX and mY are monstru's location. I don't know their internal names, so I am using mX and mY as place holders .

Then a quick change to function trage's prototype partly for aesthetics and partly to eliminate a naming conflict later. Using the same name for multiple things makes it hard to find stuff.

Using x in multiple functions is cool. Using two things named x in the same function's pretty dangerous even if scoped differently so they don't conflict. Using x x; variable x of type x is just messy, so vector<monstru> monstru should be cleaned up to prevent confusion and screw-ups.

void trage(vector<monstru>& monstruList,
           vector<vector<char> > &map,
           int x,
           int y,
           int face)
{
    if (face == 1) // not sure what this does, but I'm keeping it.
    {
        auto it = std::find_if(monstruList.begin(),
                               monstruList.end(),
                               [x,y](const monstru & monst) // lambda expression
                               {
                                   return monst.isAt(x,y);
                               });

        if (it != monstruList.end())
        {
            monstruList.erase(it);
        }
    }
}

The lambda expression is sort of an embedded anonymous function. But only sort of. No point re-explaining it, so read this: What is a lambda expression in C++11?

[x,y] says that ther local x and y variables are 'captured' and available inside the body of the lambda expression.

(const monstru & monst) is the parameterlist, specifically the lambda is called with a reference to a constant monstru that we'll call monst.

The body is dead simple, it just returns the result of the isAt method I added above called with the x and y captured from the surrounding function.

The OP should also look into other ways to store the monster's location, such as a sparse matrix or a std::map.

bricklore · Answer 2 · 2015-08-28T19:23:18.797

The delete keyword has actually nothing to do with a std::vector.
It is used to delete memory which has been allocated using the new statement,
which apparently is not what you did here.

To remove an element out of an vector you should use monstru.erase(monstru.begin() + n); Which erases the nth element from the vector.

I think what you intended to do is removing the monster which matches your if-statement from the vector. But then you have to take care of not directly accessing monstru[3] for example, as it's unclear if it currently exists. So you should generalize your code with a for loop, which even makes it easier to read and also smaller as a nice side effect):

void trage(vector<monstru>& monstru, vector< vector<char> > map,int x,int y,int face){
    if (face == 1)
    {
        for (int i = y; i < 10; i++){
            for (int m = 0; m < monstru.size(); m++){
                //use index 'm' here:
                if (monstru[m].GetX() == x && monstru[0].GetY() == i){
                    monstru.erase(monstru.begin() + m);
                    cout << "Ai ucis un monstru" << endl;
                }
            }
        }
    }
}

Right idea, but there is little point to the outer for loop since you're iterating the entire array with the inner for loop. — user4581301, Aug 29 '15 at 00:16

score 0 · Answer 3 · answered Aug 28 '15 at 19:15

As you can see here, monstru.erase() need an iterator. you need to do something like

monstru.erase(monstru.begin() + index)

with index the emplacement of the monster you whant to "kill" in the vector

also, in your first snippet, a do..while might be better

Delete an object from a vector of objects

3 Answers3