Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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:

  1. Is there a better container choice than std::vector to hold Cards? If so, why?
  2. 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?
  3. 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?
  4. 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?
  5. 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?
  6. 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;
};

// Operators
bool 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();
};

// Operators
std::ostream& operator <<(std::ostream&, const CardStack&);

// Functions
void card_transfer(CardStack &source, CardStack &dest); // transfer all cards to end of dest
void 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();
};

// Functions
void draw_card(Deck&, CardStack&, unsigned quantity = 1);

#endif

cards.cpp:

#include <iostream>
#include <vector>
#include <array>
#include <algorithm>
#include <iterator>
#include <string>

#include "cards.h"

using 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);
}
share|improve this question
add comment

2 Answers

up vote 6 down vote accepted

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<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();
}
share|improve this answer
    
Thanks a lot for your input. Follow-up: (i) Could you elaborate your answer to my second question (why I should be storing by value?) (ii) You mentioned I should not expose the cards by returning an iterator, but you then go on to use typedefs to define iterators and then expose them. I'm a little confused on how that is different. Are you saying I shouldn't expose iterators or not? (iii) If CardStack::cards is public, doesn't that also expose the cards? However, if I make it private, how can I do things like transferring cards when I can't access cards of the destination? Thanks. –  kamek Jan 8 '13 at 13:44
    
The question is why would you not store them by value? If you use pointers then you introduce a whole new set of problems with managing the pointers. Unless the object is extremely expensive to copy the default action should be by value (if it is expensive then you can design a wrapper class). C++11 has a new implace push_back so you don't actually get a copy when inserting into the vector it is constructed in place so even for big objects this problem is mitigated. –  Loki Astari Jan 8 '13 at 16:19
    
You missed the point on iterator: You are exposing std::vector<Card>::iterator. This means that anybody using the class is now has to use std::vector which ties you to the implementation of using a vector (if you change then everybody using your code must also be manually changed). The alternative is to expose an iterator that is relative to the your class. My code exposes the iterator CardStack::iterator. Now people using the code are not tied to an implementation. If I change the internal representation to a std::list<Card> a simple re-compile of theor code will update it correctly. –  Loki Astari Jan 8 '13 at 16:24
    
You can still write transferring_cards() with a private version of cards by making transfer_cards() a friend of the class (or by making it a member method). You should practically never expose member variables as public (or protected). –  Loki Astari Jan 8 '13 at 16:26
    
Thank you very much for your help. I understand your point about the iterator. However, did you also mean that I shouldn't be exposing an iterator in the first place because that would allow users of the class to change the private member variable cards? Thanks again for your help. –  kamek Jan 8 '13 at 18:33
show 1 more comment

With regards to containers to store your cards in:

If you know that there will only be a set number (at compile time) of cards you might prefer std::array over std::vector, as std::array has slightly less overhead then vector, but cannot be resized.

If you know that you always want to draw from the top of the deck and never draw from the middle or the bottom you might consider the container wrapper std::queue which enforces FIFO insertion and removal from the container. The default container wrapped by std::queue is std::deque which is optimized for insertion and removal at both ends of the container.

If your top priority is ensuring that all of the cards in the deck are unique you might consider using std::set which enforces that all elements are unique, but then you don't really have control over the order of the cards in the deck. Another way to achieve card uniqueness would be to use the std algorithms for unique such as std::unique.

If you plan on adding and removing cards from the middle of the deck frequently, you might consider using std::list, but unless you're trying to implement some strange form of dealing cards (say for a magic trick simulation) I can't imagine why that would be a priority.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.