Card Deck class for a Poker game - version 2

Question

This code was first critiqued here (without the Card class): Card Deck class for a Poker game

After learning more about data structures both online and in class, I wanted to revisit my Card and Deck classes and see if I can still make them better. Based on my driver tests, they are doing nearly everything I want them to do.

However, I'm not sure if all my operations between these two classes and the interface are proper, even though they do give proper results.

For instance:

1. Is it still (theoretically) okay to return a blank Card if there are no more cards to return from the empty Deck?

2. Do I still need to maintain my Rule of Three, even though I don't do any memory allocation in either class?

These are my main concerns, but I'd like a review of all the code in case there's something I've overlooked and/or if some parts can be cleaned.

Card.h

#ifndef CARD_H
#define CARD_H

#include <iostream>
#include <string>

class Card
{
private:
    unsigned rankValue;
    char rank;
    char suit;
    std::string card;

public:
    Card();
    Card(char, char);
    Card::Card(const Card&);
    Card::~Card();
    char getRank() const {return rank;}
    char getSuit() const {return suit;}
    std::string getCard() const {return card;}
    bool operator<(const Card &rhs) const {return (rank < rhs.rank);}
    bool operator>(const Card &rhs) const {return (rank > rhs.rank);}
    bool operator==(const Card &rhs) const {return (suit == rhs.suit);}
    bool operator!=(const Card &rhs) const {return (suit != rhs.suit);}
    Card& operator=(const Card &obj);
    friend std::ostream& operator<<(std::ostream&, const Card&);
};

#endif

Card.cpp

#include "Card.h"

Card::Card() : rankValue(0), rank('*'), suit('*'), card("**") {}

Card::Card(const Card &obj)
{
    rankValue = obj.rankValue;
    rank = obj.rank;
    suit = obj.suit;
    card = obj.card;
}

Card::~Card() {}

Card::Card(char rank, char suit)
{
    this->rank = rank;
    this->suit = suit;
    card += rank;
    card += suit;

    if (rank == 'A')
        rankValue = 1;
    else if (rank == '2')
        rankValue = 2;
    else if (rank == '3')
        rankValue = 3;
    else if (rank == '4')
        rankValue = 4;
    else if (rank == '5')
        rankValue = 5;
    else if (rank == '6')
        rankValue = 6;
    else if (rank == '7')
        rankValue = 7;
    else if (rank == '8')
        rankValue = 8;
    else if (rank == '9')
        rankValue = 9;
    else if (rank == 'T')
        rankValue = 10;
    else if (rank == 'J')
        rankValue = 11;
    else if (rank == 'Q')
        rankValue = 12;
    else if (rank == 'K')
        rankValue = 13;
}

Card &Card::operator=(const Card &obj)
{
    // if not self-assignment and lhs is a Blank card
    if (this != &obj && this->card == "**")
    {
        this->card = obj.card;
    }

    return *this;
}

std::ostream& operator<<(std::ostream &out, const Card &aCard)
{
    out << '[' << aCard.card << ']';
    return out;
}

Deck.h

#ifndef DECK_H
#define DECK_H

#include <array>
#include "Card.h"

class Deck
{
private:
    static const unsigned MAX_SIZE = 52;
    int topCardIndex;
    std::array<Card, MAX_SIZE> cards;

    void build();

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

#endif

Deck.cpp

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

Deck::Deck() : topCardIndex(-1)
{
    build();
    shuffle();
}

void Deck::build()
{
    const unsigned NUMBER_OF_RANKS = 13;
    const unsigned NUMBER_OF_SUITS = 4;
    const char RANKS[NUMBER_OF_RANKS] = {'A','2','3','4','5','6','7','8','9','T','J','Q','K'};
    const char SUITS[NUMBER_OF_SUITS] = {'H','D','C','S'};

    for (unsigned rank = 0; rank < NUMBER_OF_RANKS; ++rank)
    {
        for (unsigned suit = 0; suit < NUMBER_OF_SUITS; ++suit)
        {
            Card newCard(RANKS[rank], SUITS[suit]);
            topCardIndex++;
            cards[topCardIndex] = newCard;
        }
    }
}

void Deck::shuffle()
{
    topCardIndex = MAX_SIZE-1;
    std::random_shuffle(&cards[0], &cards[topCardIndex]);
}

Card Deck::deal()
{
    if (empty())
    {
        std::cerr << "\nDECK IS EMPTY\n";
        Card blankCard;
        return blankCard;
    }

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

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

    return out;
}

Answer 1

To add to the already suggested improvements:

You have:

bool operator<(const Card &rhs) const {return (rank < rhs.rank);}

This does not sound right to me.

If you ever decide to create a set of Cards or sort a collection of Cards, I assume you are going to rely on this function. In order to sort a collection of Cards, you have to answer the following questions:

1. Is an Ace is to be put before a Two or after a King?
2. Is an Ace of Diamonds to be equal in ordering to an Ace of Clubs?
3. Is Two of Diamonds to be less or greater than Three of Clubs?

Assuming the following answers:

1. An Ace is to be put before a Two.
2. Use the following order of Suites: Clubs, Diamonds, Hearts, Spades (the ordering used in the game Bridge). By that logic, Ace of Diamonds is greater than an Ace of Clubs.
3. Using the ordering used in Bridge, Two of Diamonds is greater than Thee of Clubs.

You'll have to update your operator< function to:

bool operator<(const Card &rhs) const
{
   if ( this->suite != rhs.suite )
   {
      // Suites are 'C', 'D', 'H', and 'S'.
      // Lucky coincidence.
      return (this->suite < rhs.suite);
   }

   // Need to use rankValue, which are ordered 1-13, not rank.
   return (this->rankValue < rhs.rankValue);
}

Also, I would change the implementation of the constructor

Card::Card(char rank, char suit)

to

Card::Card(char rank, char suit) : rank(rank),
                                   suit(suit),
                                   rankValue(getRankValue(rank))
                                   // Initialize members in
                                   // the initializer list whenever possible. 
{
}

And move the logic of getting the ordered rank value from the input rank to a helper function. In the function, use a switch instead of an if-elseif logic.

static int Card::getRankeValue(int rank)
{
    switch (rank)
    {
       case 'A':
          return 1;
       case '2':
       case '3':
       case '4':
       case '5':
       case '6':
       case '7':
       case '8':
       case '9':
          return rank-'0';
       case 'T':
          return 10;
       case 'J':
          return 11;
       case 'Q':
          return 12;
       case 'K':
          return 13;
       default:
          return -1;
    }
}

Answer 2

No pointing defining methods where the compiler generated versions does the same:

Card::Card(const Card &obj)
{
    rankValue = obj.rankValue;
    rank = obj.rank;
    suit = obj.suit;
    card = obj.card;
}

Card::~Card() {}

Card &Card::operator=(const Card &obj)
{
    // if not self-assignment and lhs is a Blank card
    if (this != &obj && this->card == "**")
    {
        this->card = obj.card;
    }

    return *this;
}

You can get rid of both the above. The compiler generated versions work fine.

For the assignment operator. Why care if it is self assignment. In this case it makes no difference. But why are you only copying the string. Don't you want to copy all the members?

In Card::Card(char rank, char suit)

There is no range checking for rank and suit.
What happens to rankValue if I call:

 Card c('X', '4');

With simple output operators I like to make it a single line:

out << '[' << aCard.card << ']';
return out;

// can be:

return out << '[' << aCard.card << ']';

Answer 3

• There's no reason to have a card member in Card. The other fields already make up this type, so it's quite redundant. With this removed, <string> can be removed from the header as well.

• MAX_SIZE could be of type std::size_t since it's a size.

• Since std::array is accessible, it should replace all the C-arrays in build().

• With C++11, std::random_shuffle() should be replaced with std::shuffle(), which is found in <random>. Its randomization is considered an improvement over that of std::rand().

• It's not necessary for deal() to display something. The calling code should display an instead error if needed.

• It may be better to have a separate size member instead of using topCardIndex to maintain the current size. This would also make size() and empty() more readable.

Answer 4

One thing I would consider doing here is to make the ranks and suits enumerations then store those in your class:

enum class rank_t: char {
    TWO = '2',
    THREE = '3',
    FOUR = '4',
    FIVE = '5',
    SIX = '6',
    SEVEN = '7',
    EIGHT = '8',
    NINE = '9',
    TEN = 'T',
    JACK = 'J',
    QUEEN = 'Q',
    KING = 'K',
    ACE = 'A'
};

enum class suit_t: char{
    CLUBS = 'C',
    DIAMONDS = 'D',
    HEARTS = 'H',
    SPADES = 'S'
};

class Card{
    rank_t rank;
    suit_t suit;
};

I think this is a more type safe way than just using characters. This also has the benefit of recovering the string representation by casting to the underlying type via static_cast.