Deck of cards program using a card value map

Question

Previous review of this project: Nested Deck class design and implementation

My program finally meets all of my expectations to the best of my ability, but I'd still like another review. Please give as many critiques as you can. I will then implement this with future games that I have in mind.

CardValuesMap.h

#ifndef CARDVALUESMAP_H
#define CARDVALUESMAP_H

std::map<char, unsigned> retrieveMap()
{
    std::map<char, unsigned> values;

    values['A'] = 1;
    values['2'] = 2;
    values['3'] = 3;
    values['4'] = 4;
    values['5'] = 5;
    values['6'] = 6;
    values['7'] = 7;
    values['8'] = 8;
    values['9'] = 9;
    values['T'] = 10;
    values['J'] = 11;
    values['Q'] = 12;
    values['K'] = 13;

    return values;
}

#endif

Deck.h

#ifndef DECK_H
#define DECK_H

#include <iostream>
#include <array>
#include <map>
#include <string>

class Deck;

class Card
{
private:
    friend class Deck;
    unsigned value;
    char rank;
    char suit;
    Card::Card(unsigned v, char r, char s) : value(v), rank(r), suit(s) {}

public:
    Card::Card() : value(0), rank(' '), suit(' ') {}
    bool operator<(const Card &rhs) const {return (value < rhs.value);}
    bool operator>(const Card &rhs) const {return (value > rhs.value);}
    bool operator==(const Card &rhs) const {return (suit == rhs.suit);}
    bool operator!=(const Card &rhs) const {return (suit != rhs.suit);}
    friend std::ostream& operator<<(std::ostream &out, const Card &aCard)
        {return out << '[' << aCard.rank << aCard.suit << ']';}
};

class Deck
{
private:
    static const unsigned MAX_SIZE = 52;
    static const std::map<char, unsigned> values;
    static const std::string SUITS;
    std::array<Card, MAX_SIZE> cards;
    int topCardPos;

public:
    Deck();
    void shuffle();
    Card deal();
    unsigned size() const {return topCardPos+1;}
    unsigned empty() const {return topCardPos == -1;}
    friend std::ostream& operator<<(std::ostream &out, const Deck &aDeck);
};

#endif

Deck.cpp

#include "Deck.h"
#include "CardValuesMap.h"
#include <algorithm>

const std::map<char, unsigned> Deck::values = retrieveMap();
const std::string Deck::SUITS = "HDCS";

Deck::Deck() : topCardPos(-1)
{
    for (auto valueIter = values.cbegin(); valueIter != values.cend(); ++valueIter)
    {
        for (auto suitIter = SUITS.cbegin(); suitIter != SUITS.cend(); ++suitIter)
        {
            topCardPos++;
            cards[topCardPos] = Card(valueIter->second, valueIter->first, *suitIter);
        }
    }

    shuffle();
}

void Deck::shuffle()
{
    topCardPos = MAX_SIZE-1;
    std::random_shuffle(&cards.front(), &cards.back()+1);
}

Card Deck::deal()
{
    if (empty())
    {
        std::cerr << "\nDECK IS EMPTY -- RESHUFFLING NOW\n";
        shuffle();
    }

    topCardPos--;
    return cards[topCardPos+1];
}

std::ostream& operator<<(std::ostream &out, const Deck &aDeck)
{
    for (int i = aDeck.size()-1; i > -1; --i)
    {
        out << "\n" << aDeck.cards[i];
    }

    return out;
}

Answer 1

I haven't followed your previous reviews, so a few of these may be things that have already been pointed out.

Anyway, here's my go at it.

---

The Card:: is invalid on the following:

Card::Card(unsigned v, char r, char s) : value(v), rank(r), suit(s) {}
Card::Card() : value(0), rank(' '), suit(' ') {}

---

The suit should likely be an enum (in particular, a strongly typed one as you're using C++11).

---

The comparators in Card are non-sensical. Why do you sometimes compare value and sometimes compare suit? How are a Jack of Spades and a 2 of Spades equal just because they're both a Spade?

---

For maintainability, try to define comparators in terms of each other: bool operator!=(const Card &rhs) const {return !(suit == rhs.suit);} and so on.

---

Don't use bare unsigned. Use unsigned int.

(Personal preference really on this one)

---

Even better yet, define the types as public typedefs. Like Deck::size_type, Card:suit_type and so on (standard library-esque).

---

Why make the maximum size be 52?

You could either have the size be templated and default to 52, or you could have it be a constructor parameter (in which case you'd have to use a vector rather than std::array).

---

Is the mapping in retrieveMap ever meant to be used by consuming code? If not, make it static in Deck.cpp.

---

Card::deal has no business outputting anything. If dealing can't be done, throw an exception.

---

Rather than having to think in reverse iteration, you could think of the top of the deck as being 0. This would likely simplify a lot of the logic.

---

Are you happy being stuck with a 52 card deck? What if you don't want Jokers? What if you want some completely different kind of card system? A

Basically what I'm saying is that a deck shouldn't be responsible for filling itself up with cards.

In fact, constructing objects in a construct tends to be an anti-pattern: http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/. Note that "new" in this context is talking about Java, not C++. Though the terminology is Java centric, what it actually means is is construction in general. Constructing objects in a construct tends to imply that the objects should be provided to the constructor rather than be created in there.

Rather than having the deck populate itself with a traditional set of cards, have a utility method:

Deck classDeck = Deck::makeClassicDeck();

This lets you have the convenience that you currently have, but it doesn't hold you to a certain set of cards (you could also go the two constructors route).

---

Why are you trying to hide the Card class? You should probably trust your consuming code to handle cards. How do you know that the code isn't going to be used for some very odd game that involves a deck composed of a normal deck and 6 extra jokers? If you let the consumer of Deck decide what to put it in, it's a lot more flexible.

---

As a person about to make a program that involves card game logic, why should I use your Deck class? What does it offer me over using a simple std::deqeue (or std::stack or std::vector) of Card? The way I see it at the moment, it offers basically nothing over a vector. It just has a more convenient shuffle.

---

MAX_SIZE should be a std::size_t

empty should return a bool

size should return a std::size_t

---

I'm not sure if I'd bother defining an ostream<<. Might be more useful to let each application determine how it wants to print a deck rather than tie it to one format.

---

I'd go with a more idiomatic shuffling approach:

std::random_shuffle(cards.begin(), cards.end());

---

I don't like SUITS. Why is it the only all caps variable?

I don't like aDeck and aCard. A const Deck& is obviously a Deck. Just call it deck or something. Names beginning with an article seem odd (imagine if Deck::cards were instead theCards).

---

Since it's private, it's not as important, but Card::Card(unsigned v, char r, char s) should still have proper names for the parameters.

---

Crazy paranoid moment: deal() is not exception safe (well, technically it is since Card::Card never throws, but there's no hard gaurantee of that in place).

You could theoretically encounter an exception in the card constructor which would mean the card would properly be removed from the deck, but the consuming code would not be able to have it.

Then again, this is complete paranoia, especially since Card's constructor never throws for the time being.

(Just happened to think of it since the oddity of std::stack's pop() returning void has always been an interesting little necessity to me.)