CODEWITHSUNDEEP
c++
Bacu
Bacu

Reputation: 539

Vector made up of user defined type can't use vector::erase();

Trying to make a simple card game program. I'm having trouble with the vector::erase(); function on a vector made of of type Card. It seems that it can't use it because there isn't an overloaded function in vector::erase(); that deals with a <Card>. This confuses me because a vector is templated. 

#include <iostream>
#include <vector>
#include <algorithm>

enum card_suit {SPADE, CLUB, DIAMOND, HEART};

class Card;
class Player;
class Deck;

class Card
{
    unsigned short rank;
    card_suit suit;
public:
    Card(unsigned short r, card_suit s)
    {
        rank = r;
        suit = s;
    }
    bool operator == (Card& a)
    {
        if(this->rank == a.rank && this->suit == a.suit) return true;
        else return false;
    }
};

class Deck
{
    std::vector<Card> cards;
public:
    Deck& add_card(Card c)
    {
        cards.push_back(c);
        return *this;
    }

    Deck& remove_card(Card c)
    {
        for(std::vector<Card>::iterator i=cards.begin(); i<cards.end(); i++)
        {
            if(*i==c) cards.erase(cards.begin()-i);
        }
        return *this;
    }
    Deck& shuffle()
    {
    }
};

class Player
{
    Deck hand;
    unsigned short points;
public:
    Player()
    {
        points=0;
    }
};


int main()
{

    return 0;
}

Any idea what to do?

Upvotes: 1

Views: 1776

Answers (4)

Loki Astari
Loki Astari

Reputation: 264391

The problem is:

cards.begin() - i
    //        ^^^^

The result of this expression is an ptr_diff.
While the erase() method takes an iterator.

Maybe you meant:

cards.erase( i );

Upvotes: 0

eugene_che
eugene_che

Reputation: 1997

If I do not miss something you just need to replace cards.erase(cards.begin()-i); with cards.erase(i);. Really I cannot understand what do you mean by that code.

Well, if to speak about effectiveness I guess that the card order makes no sense and we can use this feature:

std::vector<Card>::iterator i = std::find( cards.begin(), cards.end(), c );
if ( i != cards.end() ) {
    if ( i != (cards.end()-1) )
        *i = *(cards.end()-1);
    cards.pop_back();
}

*The suggested erase-remove combination results in copies of the vector tail.

Upvotes: 1

Kerrek SB
Kerrek SB

Reputation: 477040

This should generally work, provided you get the basic syntax and idioms right:

for (std::vector<Card>::iterator it = cards.begin(), end = cards.end(); it != end; ++it)
{
  if (c == *it)
  {
    cards.erase(it);
    break;
  }
}

Note that erasing from the container invalidates iterators, so we can only erase one element. We can't use the usual erase(it++) here, because all iterators beyond the erased one are invalidated by the erase.

In order to efficiently remove all matching elements from a vector, and you should instead use the remove+erase idiom:

cards.erase(std::remove(cards.begin(), cards.end(), c), cards.end());

This first reorders the elements of the vector so that all those that match c are at the end (remove), and then (efficiently) erases that end from the vector (erase).

Upvotes: 4

littleadv
littleadv

Reputation: 20272

If you want to remove just one card, then instead of this

        if(*i==c) cards.erase(cards.begin()-i);

do this:

        if(*i==c) cards.erase(i);

Upvotes: 1

Related Questions