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!

Answer 1

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()

Answer 2

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().