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 <iostream>#include <vector>#include <random>#include <ctime>#include <string>/*************************** Card ***************************/struct Card{    // Data    unsigned rank;    char suit;    // Constructors    Card(unsigned r, char s) : rank(r), suit(s) {};    // Copy control    Card(const Card &c) : rank(c.rank), suit(c.suit) {}    Card& operator=(const Card &rhs) {rank = rhs.rank; suit = rhs.suit;}    // Methods    std::string to_string() const;};// Operatorsbool operator==(const Card &lhs, const Card &rhs);bool operator!=(const Card &lhs, const Card &rhs);bool operator<(const Card &lhs, const Card &rhs);bool operator>(const Card &lhs, const Card &rhs);std::ostream& operator<<(std::ostream &os, const Card &card);/*************************** CardStack ***************************/class CardStack{    private:        // Data        std::default_random_engine rand_eng;    protected:        // Methods        std::vector<Card>::iterator string_to_iter(std::string);    public:        // Data        std::vector<Card> cards;        // Constructors        CardStack() {rand_eng.seed(time(0));}        // Methods        std::size_t size() const {return cards.size();}        CardStack& shuffle();        CardStack& sort_rank();};// Operatorsstd::ostream& operator <<(std::ostream&, const CardStack&);// Functionsvoid card_transfer(CardStack &source, CardStack &dest); // transfer all cards to end of destvoid card_transfer(CardStack &source, CardStack &dest, Card&); // transfer specific card to end of dest/*************************** Deck : public CardStack ***************************/class Deck : public CardStack{    friend void draw_cards(Deck&, CardStack&, unsigned quantity);    public:        // Constructors        Deck();        // Methods        Card& top();};// Functionsvoid draw_card(Deck&, CardStack&, unsigned quantity = 1);#endifcards.cpp:#include <iostream>#include <vector>#include <array>#include <algorithm>#include <iterator>#include <string>#include cards.husing std::vector;using std::array;using std::string;const array<unsigned, 13> RANKS = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13};const array<char, 4> SUITS = {'c', 'd', 'h', 's'};/*************************** Card ***************************/string Card::to_string() const{    string r;    switch (rank)    {        case 1:            r = A; break;        case 10:            r = T; break;        case 11:            r = J; break;        case 12:            r = Q; break;        case 13:            r = K; break;        default:            r = std::to_string(rank);    }    return r + suit;}bool operator==(const Card &lhs, const Card &rhs) {return lhs.rank == rhs.rank && lhs.suit == rhs.suit;}bool operator!=(const Card &lhs, const Card &rhs) {return !(lhs == rhs);}bool operator<(const Card &lhs, const Card &rhs) {return lhs.rank < rhs.rank;}bool operator>(const Card &lhs, const Card &rhs) {return !(lhs < rhs);}std::ostream& operator<<(std::ostream &os, const Card &card){    os << card.to_string();    return os;}/*************************** CardStack ***************************/vector<Card>::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;});}CardStack& CardStack::shuffle(){    std::shuffle(cards.begin(), cards.end(), rand_eng);    return *this;}CardStack& CardStack::sort_rank(){    std::sort(cards.begin(), cards.end());    return *this;}std::ostream& operator<<(std::ostream &os, const CardStack &cs){    for (auto c : cs.cards)        os << c <<  ;    return os;}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);    }}void card_transfer(CardStack &source, CardStack &dest, Card &card){    auto it = std::find(source.cards.begin(), source.cards.end(), card);    if (it != source.cards.end())    {        dest.cards.push_back(*it);        source.cards.erase(it);    }}/*************************** Deck : public CardStack ***************************/Deck::Deck(){    for (const auto &r : RANKS)        for (const auto &s : SUITS)            cards.emplace_back(r, s);}Card& Deck::top(){    return cards.front();}void draw_cards(Deck &deck, CardStack &cs, unsigned quantity = 1){    // Copy cards from deck to specified card stack    std::copy(deck.cards.begin(), deck.cards.begin() + quantity, std::back_inserter(cs.cards));    // Erase cards from deck    deck.cards.erase(deck.cards.begin(), deck.cards.begin() + quantity);}
Playing cards in C++
c++;classes;c++11;playing cards
AnswersIs 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);Reviewstruct 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:        // MethodsExposing the iterator type here locks you into that type. I would define my own iterator type locally to the CardStack (see below).        std::vector<Card>::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<Card> 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<Card>    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<Card>::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<Card>::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);    }}Given the sizes its not a real big deal. But I would have done:void card_transfer(CardStack &source, CardStack &dest){    dest.insert(dest.cards.end(), source.cards.begin(), source.cards.end());    source.clear();}