Deleting user-defined elements in the middle of a vector

Question

I'm coding a program where I want to draw a card, and then delete so that it doesn't get drawn again.

I have a vector of Cards (class containing 2 structs that define Suit and Value) called deck and I don't really know how to use iterators very well, here a code snippet:

void Player::discardCard(CardDeck masterDeck)
{
    cout << "Erasing: " << masterDeck.getDeck().at(cardSelect).toString() << endl;
    /*Attempt1*/
    masterDeck.getDeck().erase(masterDeck.getDeck().begin()+cardSelect);

    /*Attempt 2*/
    vector<Card>::iterator itr;
    itr = masterDeck.getDeck().begin() + cardSelect;
    masterDeck.getDeck().erase(itr);
}

cardSelect has the location of the card I'm going to delete. It's generated randomly within the boundaries of 0 and the size of deck; therefore it shouldn't be pointing to a position out of boundaries.

Everytime I compile I get the following error:

"Expression: vector erase iterator outside range"

I really don't know what to do, hopefully someonw can help me, thanks in advance!

cardSelect has the location of the card I'm going to delete. — Jota, Apr 01 '11 at 05:14
Seems like you're passing `masterDeck` by value, maybe that's part of the problem. Otherwise, it might be an extra problem. — Fred Foo, Apr 01 '11 at 05:18
What is the definition of `CardDeck`? Also, are you sure you should be passing that by value? — GManNickG, Apr 01 '11 at 05:18
Looks like you need frequent insert and remove. using list would be great option here. — Vijay, Apr 01 '11 at 05:27
My bet is that `getDeck` returns the vector by value. It should have this signature: `vector& getDeck()`. — Yakov Galka, Apr 01 '11 at 05:57
If you're drawing a lot of cards, an easier approach would be to `std::random_shuffle` the vector and then just go through them in order. — dan04, Apr 01 '11 at 06:10

Yakov Galka · Accepted Answer · 2011-04-01T06:14:08.957

2

My bet is that getDeck returns the vector by value. It causes itr to point to and erase to operate on different copies of the vector. Thus you get the error. You should return the vector by reference. Change getDeck signature to this one:

vector<Card>& getDeck()

edited Apr 01 '11 at 06:14

answered Apr 01 '11 at 06:08

Yakov Galka

70,775
16
139
220

Tod · Answer 2 · 2011-04-01T06:20:12.703

Let me go off topic first. Your design is a little suspect. First passing in CardDeck by value is almost certainly not what you want but that's even beside the point. Why should your Player class have all this inside knowledge about the private innards of CardDeck. It shouldn't care that you store the deck as a vector or deque (ha ha), or what the structure is. It just shouldn't know that. All it knows is it wants to discard a card.

masterDeck.Discard(selectedCard);

Also note that selectedCard has to be between 0 and ONE LESS than the size of the deck, but even that's probably not your problem (although it will be 1/53rd of the time)

So to answer your question we really would need to now a little more about masterDeck. Did you implement a valid custom copy constructor? Since you're passing by value odds are good you're not correctly copying the underlying vector, in fact it's probably empty and none of the deletes will work. Try checking the size. If you don't ever want the deck copied then you can let the compiler help you by declaring a private copy constructor and then never defining it. See Scott Meyer's Effective C++ Item 11.

Finally one last piece of advice, I believe once you erase with your iterator, you invalidate it. ~~The vector might get reallocated (almost certainly will if you erase anywhere but the end).~~ I'm just telling you so that you don't try to call erase more than once on the same iterator. One of the tricky things about iterators is how easy it can be to invalidate them, which is why you often seen checks for iter != coll.end().

When you erase something the vector never reallocates. Even `vec.clear()`. Yet it invalidates the iterators past the element you erase (according to standard). — Yakov Galka, Apr 01 '11 at 06:06
@ybungalobill Thanks. So I was right for the wrong reasons? I must be getting tired, I thought I looked it up to be sure but I misread it, "Inserting or Removing elements invalidates...iterators. If an insertion causes reallocation..." I'll fix my answer. — Tod, Apr 01 '11 at 06:15

score 0 · Answer 3 · answered Apr 01 '11 at 06:05

0

"It's generated randomly within the boundaries of 0 and the size of deck".

The valid range should be "between 0 and the size of the deck minus 1". This could generate range error at run time.

answered Apr 01 '11 at 06:05

Keith

6,756
19
23

Deleting user-defined elements in the middle of a vector

3 Answers3