Playing cards in C++

I am currently learning C++ (note: I am using C++11) and have begun working on a small project to practice what I've been learning. This project is a deck of cards that I hope to use later to create simple card games. I have created three classes: Card represents a single playing card, CardStack represents a stack of cards (e.g., to be used for a "hand" or "discard pile", etc.), and Deck is a sub-class of CardStack that contains all 52 cards in a standard deck of cards.

In addition to a general review of my code, I am specifically looking for feedback/answers to the following:

• Is there a better container choice than std::vector to hold Cards? If so, why?

• Should I be storing a pointer to Card rather than copies of Card in CardStack::cards? If so, should I use std::shared_ptr or std::unique_ptr?

• How do you feel about my choice to make card_transfer and draw_card functions rather than members of CardStack and Deck, respectively? Does Deck::draw_card(CardStack&, unsigned) make more sense? Why?

• My method to move Cards from one CardStack to another right now is to copy it over than erase it from the source. I understand C++11 introduced move semantics. Is this the kind of place this type of behaviour should be implemented?

• Obviously only one copy of a Card should exist at any time (if this is a standard deck of cards). How can I go about enforcing this?

• I included the method CardStack::string_to_iter to facilitate input from the user in selecting a card (i.e., he or she can enter 'Ts' for the ten of spades). Is there anything wrong with how I did this? I decided that input validation would be done on the game level, and this method assumes valid input. Is this a good idea?

cards.h:

```
#ifndef CARDS_H
#define CARDS_H

#include
#include
#include
#include
#include

/***
Card
***/
struct Card
{
// Data
unsigned rank;
char suit;
// Constructors

Answers

Is there a better container choice than std::vector to hold Cards? If so, why?

Depends.

Should I be storing a pointer to Card rather than copies of Card in CardStack::cards?

No. You should be storing by value.

If so, should I use std::shared_ptr or std::unique_ptr?

No. But the choice would depend on how you define ownership.

How do you feel about my choice to make card_transfer and draw_card functions rather than members of CardStack and Deck, respectively?

Don't like it. But it may work. The principle of "Separation of Concerns" your class should either "business logic (card stuff)" or "Resource Management (card container)" The question you need to decide is if CardStack is doing business logic or resource management.

Does Deck::draw_card(CardStack&, unsigned) make more sense? Why?

I think so: This way you are hiding the implementation but providing a neat interface.

My method to move Cards from one CardStack to another right now is to copy it over than erase it from the source. I understand C++11 introduced move semantics. Is this the kind of place this type of behaviour should be implemented?

No. Move semantics leaves the source in an undefined state (this is not what move semantics are for). Move is used to move an object efficiently to a destination where the source is no longer valid.

Obviously only one copy of a Card should exist at any time (if this is a standard deck of cards). How can I go about enforcing this?

Not sure. But I don't think its a big problem.

I included the method CardStack::string_to_iter to facilitate input from the user in selecting a card (i.e., he or she can enter 'Ts' for the ten of spades). Is there anything wrong with how I did this? I decided that input validation would be done on the game level, and this method assumes valid input. Is this a good idea?

I would have defined an input operator std::istream& operator>>(std::istream& s, Card& c);
Review

struct Card
{
    // Data
    unsigned rank;
    char suit;
    // Constructors
    Card(unsigned r, char s) : rank(r), suit(s) {};

Default version of Copy constructor and assignment operator already do this:

// Copy control
    Card(const Card &c) : rank(c.rank), suit(c.suit) {}
    Card& operator=(const Card &rhs) {rank = rhs.rank; suit = rhs.suit; /*You forgot the return *this;*/}

Sure this is fine.

But (personally) I would not bother.

// Methods
    std::string to_string() const;
};

class CardStack
{
    private:
        // Data
        std::default_random_engine rand_eng;

Protected does not really give you much.

Stroustrup even indicated that he now thinks this was a mistake.

To access it all I need to do is inherit from your class. If I really want I can then expose it publicly for others. Thus it really provides no protection.

protected:
        // Methods

Exposing the iterator type here locks you into that type. I would define my own iterator type locally to the CardStack (see below).

std::vector::iterator string_to_iter(std::string);
    public:

Here you are exposing the cards. Thus allows the internal state to be changed. This is a no no. Lock up the interface only allow the state to be changed via a very specific closed interface (also note you are locking yourself to vector).

// Data
        std::vector cards;

        // Constructors
        CardStack() {rand_eng.seed(time(0));}
        // Methods
        std::size_t size() const {return cards.size();}
        CardStack& shuffle();
        CardStack& sort_rank();
};

I would have defined the Iterator type like this:

class CardStack
{
        typedef  std::vector    Container;
    public:
        typedef  Container::iterator        iterator;
        typedef  Container::const_iterator  const_iterator;
};

Now you expose an iterator type. But the user of the iterator can not assume anything about it or the underlying container type.

Not sure this is a good idea:

bool operator<(const Card &lhs, const Card &rhs) {return lhs.rank < rhs.rank;}

If you use Card as the key in a map or set then things will break. As you do not define a Strict Weak ordering.

Small thing:

vector::iterator CardStack::string_to_iter(string str) 
{
    // Returns iterator to specified card in cards, otherwise returns cards.end()
    return std::find_if(cards.begin(), cards.end(), [str] (const Card &c) {return c.to_string() == str;});
}

Pass the str by const reference.

vector::iterator CardStack::string_to_iter(string const& str) 
{
    // Returns iterator to specified card in cards, otherwise returns cards.end()
    return std::find_if(cards.begin(), cards.end(), [str const&] (const Card &c) {return c.to_string() == str;});
}

This is not very efficient:

```
void card_transfer(CardStack &source, CardStack &dest)
{
while (source.cards.size() > 0)
{
auto it = source.cards.begin();
dest.cards.push_back(*it);
source.cards.erase(it);

StackExchange Code Review Q#20254, answer score: 6