5

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!

Improve this question
7
  • cardSelect has the location of the card I'm going to delete. Commented Apr 1, 2011 at 5:14
  • What is cardSelect, and int, char, __int64? Commented Apr 1, 2011 at 5:17
  • 2
    Seems like you're passing masterDeck by value, maybe that's part of the problem. Otherwise, it might be an extra problem. Commented Apr 1, 2011 at 5:18
  • What is the definition of CardDeck? Also, are you sure you should be passing that by value? Commented Apr 1, 2011 at 5:18
  • 1
    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. Commented Apr 1, 2011 at 6:10

3 Answers 3

Reset to default
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()
Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

1

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

Improve this answer

2 Comments

When you erase something the vector never reallocates. Even vec.clear(). Yet it invalidates the iterators past the element you erase (according to standard).
@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.
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.

Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.