Nested Deck class design and implementation

Question

Previous review of this project:

Deck and Card classes and member-accessing with one header

I'm nearly finished with my deck of cards project, and this time I made changes to hide the Card class by nesting it in the Deck class. I'm still getting correct results when displaying the deck. However, I do now realize that I can no longer maintain Card objects outside the class. Instead, I have to call Card deal() with a Deck object in order to retrieve a Card, and I cannot store that Card anywhere.

Does this render my design ineffective for this purpose? If so, how else can I maintain Cards outside the class while still preventing the user from directly constructing a Card? Would a private constructor (with Card un-nested from Deck) be a better option?

Deck.h

#ifndef DECK_H
#define DECK_H
#include <iostream>
#include <array>
class Deck
{
private:
 class Card
 {
 private:
 char rank;
 char suit;
 unsigned value;
 public:
 Card::Card() : value(0), rank(' '), suit(' ') {}
 Card::Card(char r, char s, unsigned v) : rank(r), suit(s), value(v) {}
 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);
 friend std::ostream& operator<<(std::ostream &out, const Card &aCard)
 {return out << '[' << aCard.rank << aCard.suit << ']';}
 };
 static const unsigned VALUES[13];
 static const char RANKS[13];
 static const char SUITS[4];
 static const unsigned MAX_SIZE = 52;
 std::array<Card, MAX_SIZE> cards;
 int topCardPos;
public:
 Deck();
 void shuffle();
 Card deal();
 unsigned size() const {return topCardPos+1;}
 bool empty() const {return topCardPos == -1;}
 friend std::ostream& operator<<(std::ostream &out, const Deck &aDeck);
};
#endif

Deck.cpp

#include <algorithm>
#include "Deck.h"
const unsigned Deck::VALUES[13] = {1,2,3,4,5,6,7,8,9,10,10,10,10};
const char Deck::RANKS[13] = {'A','2','3','4','5','6','7','8','9','T','J','Q','K'};
const char Deck::SUITS[4] = {'H','D','C','S'};
Deck::Deck() : topCardPos(-1)
{
 for (unsigned rank = 0, value = 0; rank, value < 13; ++rank, ++value)
 {
 for (unsigned suit = 0; suit < 4; ++suit)
 {
 topCardPos++;
 cards[topCardPos] = Card(RANKS[rank], SUITS[suit], VALUES[value]);
 }
 }
 shuffle();
}
void Deck::shuffle()
{
 topCardPos = MAX_SIZE-1;
 std::random_shuffle(&cards.front(), &cards.back()+1);
}
Deck::Card Deck::deal()
{
 if (empty())
 {
 std::cerr << "\nDECK IS EMPTY\n";
 Card blankCard;
 return blankCard;
 }
 topCardPos--;
 return cards[topCardPos+1];
}
std::ostream& operator<<(std::ostream &out, const Deck &aDeck)
{
 for (unsigned iter = aDeck.size(); iter--> 0;)
 {
 out << "\n" << aDeck.cards[iter];
 }
 return out;
}

Answer 1

The way I understand your question, you want Card to be publicly available, yet only Deck should be able to construct a Card. One way to achieve this is to make the Card() constructor private and a friend of Deck:

class Deck;
class Card {
 Card() {}
 friend class Deck;
public:
 // ...
};
class Deck {
 Card card; // ok
public:
 Card createCard() { // ok
 return Card();
 }
};
int main()
{
 Deck deck;
 Card card = deck.createCard();
 Card doesntWork; // error! Attempt to access private member
}

A somewhat cleaner solution would be to have only createCard() as friend of Card. In that case, however, you will have to dynamically allocate the Card (and return a pointer to it), because of mutual dependencies.

(The reason for this is that Card needs the definition, and not just declaration, of Deck to declare one of the Deck functions a friend. At the same time, Deck::createCard() needs the definition of Card to be able to return a Card by value. If you return a pointer, however, you only need the declaration.)

---

Other comments:

1: Consider giving ranks, suits and values distinct and unique types. For example using enum. Make use of the type system to enlist the compiler's aid -- by doing this you will be notified if you accidentally switch types somewhere.

2: I would change

for (unsigned iter = aDeck.size(); iter--> 0;)

to the more traditional

for (int i = aDeck.size() - 1; i > -1; --i)

The latter is more readable and clear about what is going on. The former is vulnerable to be incorrected, i.e. refactored to the (incorrect)

for (unsigned iter = aDeck.size(); iter > 0; --iter)

3: Don't call a variable iter when it's a counter and not an iterator.

4: Initializer list ordering should be the same as the variables are declared in the class. By doing this, you avoid ever risking dependency problems. (Variables are always initialized in the order they are declared in the class, not the order they appear in initializer lists.)

5: Reorder your #includes in the implementation file.

Update

6: Your card generation/constructor is error-prone. Apart from introducing type safety, I would recommend generating value based on rank, for example through a function and an std::map.

• Jamal: Yes! That's my ideal main() right there. :-) I'll try to work out these changes.
• Jamal: I've made the changes (in addition to using std::vector instead of std::array), and my results are to my satisfaction. Many thanks! I have kept deal() intact, though. In that function, is there a better way to handle an empty deck (internally), or is that good enough? In its current form, I'll have to keep my default constructor.
• Lstor: @Jamal: Happy to help! You can keep it the way it is, you could reshuffle the deck and deal again or you could throw an exception. What is "best" depends on the situation. While considering this, I noticed another issue; see the updated post. By the way, consider accepting the answer if you find it satisfactory.
• Lstor: @Jamal: An object-oriented sketch to point you in the right direction: codepad.org/YRi4rgLl. If using C++11, you could instead bind the correct assignment function to a std::function, for example.
• Jamal: I got it working with an std::map, which I put in a header file. The suits is now in a string, so all of the iterating for both containers is done via the auto method.