Deck and Card classes and member-accessing with one header

Question

Previous review of this project:

Card Deck class for a Poker game - version 2

This time, I considered combining both classes into one header file so that I can reduce my use of #include and to keep them together. I also did this so that I can define my const std::string variables only once for both classes. However, I decided to try using a namespace for these variables for easier access. It makes more sense (to me) to keep them with Card and let Deck refer to them during the latter's construction, but I'm not sure how to do that.

Is this the proper thing to do, or could something be done differently? Is there a need for extern somewhere? The code below doesn't include class implementations, but I've kept them intact from the linked code.

Deck.h

#ifndef DECK_H
#define DECK_H
#include <iostream>
#include <string>
#include <array>
namespace rs
{
 const std::string RANKS = "A23456789TJQK";
 const std::string SUITS = "HDCS";
}
class Card
{
private:
 unsigned rankValue;
 char rank;
 char suit;
public:
 Card();
 Card(char r, char s);
 bool operator<(const Card &rhs) const {return (rankValue < rhs.rankValue);}
 bool operator>(const Card &rhs) const {return (rankValue > rhs.rankValue);}
 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;
 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

Answer 1

I see some things that may help you improve your code.

Consider using an enum class

Instead of using constructs such as char suit, use the superior safety of an enum class. You can read this question for an overview of when to use one, but I think this would be a better way to do things for this case:

enum class suits : char {HEART = 'H', DIAMOND = 'D', CLUB = 'C', SPADE = 'S'};
enum class ranks : char {ACE=1, TWO, THREE, FOUR, FIVE, SIX, SEVEN, 
 EIGHT, NINE, TEN, JACK, QUEEN, KING };

This allows the small storage size of a char, the convenience of an enum and the type-safety of a class.

Force only valid cards to be created

With the existing default constructor, and with the constructor that takes arguments, any enforcement on validity of initialized values becomes part of the detail of the constructor rather than part of the interface. By using the enum class types mentioned above, we can make it very clear from the interface that it's not going to be possible to create invalid cards:

Card(ranks r, suits s);

Delete unhelpful autogenerated operators

Duplicating cards will get you kicked out of most casinos. It's probably better to avoid such trouble before it begins by making it a little harder to do:

Card() = delete;
Card(Card&) = delete;

Wrap all of your objects inside a name

Rather than putting just the constants in a namespace, put your whole interface into a namespace. In some future use in which your code is used within an electronic poker game cruise ship simulator, your Card could then be distinguished from a circuit Card that might be part of the electronic game classes, and your Deck could be distinguished from the Decks of a cruise ship.

Reconsider your class design

The Deck class is really just a collection of Card objects. While you may have ideas on how to use your Deck class, it's very likely that you're going to want to have many other kinds of Card collections such as Hand or DiscardPile or Shoe (if you're simulating casino blackjack). In addition, even the notion of Deck can change if you're using it for, say, Pinochle which uses a 48-card deck rather than a 52-card deck. For that reason, it may be better to define a generic Card collection and then specialize that for the various uses.

Reconsider your comparison operators

The comparison operators for the Card class may well be valid for your particular use, in which case, you should certainly use them, but comparisons and relative valuations tend to change by the particular rules of each card game. In some games the Ace is low, and in others high. In some, such as blackjack, it can take on the values of 1 or 11. For that reason, it is likely that the comparison operators are likely to need to be changed. This suggests that they could be made virtual so that derived Card classes can override them.

Answer 2

Here's my newest solution using Yuushi's advice from my other answer's comments:

friend should be a last resort: it really breaks encapsulation badly. In this case, it's not needed. Anything that is static const can safely be made public, or if you really want, simply make some public static functions to get ranks/suits: static const std::string& ranks() { return RANKS; }.

Deck.h

// class Card:
private:
 static const std::string RANKS;
 static const std::string SUITS;
public:
 static const std::string& ranks() {return RANKS;}
 static const std::string& suits() {return SUITS;}
// class Deck...

Card.cpp

const std::string Card::RANKS = "A23456789TJQK";
const std::string Card::SUITS = "HDCS";
// everything else...

Answer 3

Okay, I'm liking this better than namespace and extern. No more global access. I've instead moved the variables into Deck while declaring Card as a friend. These variables are still accessible to Card's implementation, and main() can no longer see them.

Deck.h

// class Card...
// class Deck:
friend class Card;
private:
 static const std::string RANKS;
 static const std::string SUITS;

Deck.cpp

const std::string RANKS = "A23456789TJQK";
const std::string SUITS = "HDCS";

• Yuushi: friend should be a last resort: it really breaks encapsulation badly. In this case, it's not needed. Anything that is static const can safely be made public, or if you really want, simply make some public static functions to get ranks/suits: static const std::string& ranks() { return RANKS; }.
• Jamal: Really? Interesting. I thought member variables shouldn't be made public, period. I'll try them both and see which one I like.
• Jamal: Ah yes! This makes sense to me now. I started thinking about std::string::npos and, after reading its documentation again, I found out that it too is static const. I'm still uncomfortable with data members declared under public, so I'm using the public static functions instead.