Card and Deck classes

Question

I would like to know whether I'm going in a correct way or not in building my Poker game. So far, I've implemented card and deck classes and I would like to see your feedback about my work. Feel free to criticize the code of any regard (organization, order, comments ... etc)

card.h

#ifndef CARD_H
#define CARD_H

#include <string>

const int SUIT_MAX(4);
const int RANK_MAX(13);

class Card
{
    friend class Deck; // Deck Class needs to access to Card Class but not vice versa
public:
    explicit Card();
    explicit Card(const int &suit, const int &rank);

    std::string Card2Str() const;

private:
    int generate_suit();
    int generate_rank();
    int get_suit() const;
    int get_rank() const;
    int m_suit;
    int m_rank;
};
#endif

card.cpp

#include <stdlib.h>     /* srand, rand */
#include "card.h"
#include <iostream>


const std::string SUIT[SUIT_MAX]  = {"S", "H", "D", "C"};
const std::string RANK[RANK_MAX]  = {"2","3","4","5","6","7","8","9","10","J","Q","K","A"};


Card::Card()
{
   m_suit = generate_suit(); 
   m_rank = generate_rank();
}

Card::Card(const int &suit, const int &rank) : m_suit(suit), m_rank(rank)
{

}

int Card::generate_suit()
{
    return rand() % (SUIT_MAX-1) + 0;
}

int Card::generate_rank()
{
    return rand() % (RANK_MAX-1) + 0;
}

std::string Card::Card2Str() const
{
    return SUIT[get_suit()] + RANK[get_rank()];
}

int Card::get_suit() const
{
    return m_suit;
}

int Card::get_rank() const
{
    return m_rank;
}

deck.h

#ifndef DECK_H
#define DECK_H

#include <vector>
#include <iostream>
#include <fstream>
#include "card.h"

using namespace std;

class Deck
{ 
public:
      explicit Deck();
      void print_Deck() const;
      void getOneCard();
private:
    std::vector<Card> m_deck;

};

#endif

deck.cpp

#include <iostream>
#include "deck.h"


Deck::Deck()
{
    for (unsigned int i(0); i < SUIT_MAX; ++i)
    {
        for (unsigned int j(0); j < RANK_MAX; ++j)
        {
            Card card(i, j);
            m_deck.push_back(card);
        }
    }
}


void Deck::print_Deck() const
{
    unsigned int count(1);

    for (unsigned int i(0); i < m_deck.size(); ++i)
    {
        std::cout << m_deck[i].Card2Str() << " ";
        if ( count == 13 )
        {
          std::cout << std::endl;
          count = 0;
        }
        ++count;
    }
}

void Deck::getOneCard()
{   
    Card cd(m_deck.back().get_suit(), m_deck.back().get_rank());
    m_deck.pop_back();
    std::cout << cd.Card2Str() << std::endl;
}

main.cpp

#include <iostream>
#include <vector>
#include <stdlib.h>     /* srand, rand */
#include <time.h>       /* time */
#include <string>

#include "card.h"
#include "deck.h"

int main()
{
    srand (time(NULL));

    Deck _deck; 
    _deck.print_Deck();
    _deck.getOneCard();

    std::cout << std::endl;
    _deck.print_Deck();


    std::cout << std::endl;

    return 0;
}

Answer 1

• Prefer nullptr to NULL if you're using C++11.

• <stdlib.h> and time.h are both C libraries. Use the respective C++ libraries <cstdlib> and <ctime>. The rest are okay. You don't need to include <vector> in main.cpp.

• Since you use a storage container for your deck, you can display it using iterators:

  for (auto iter = m_deck.begin(); iter != m_deck.cend(); ++iter)
  {
      std::cout << *iter << "\n";
  }
  

  If you're using C++11, use a range-based for-loop:

  for (auto& iter : m_deck)
  {
      std::cout << iter << "\n";
  }
  

• I don't know why you're using randomness to generate a card within the class. The constructor should be given input to determine its rank and suit. A card shouldn't create itself, and you wouldn't want this with an actual card game. Remove the generate functions entirely, and instead allow the constructor to receive arguments to determine the card's value.

• You also don't need to construct the deck within its constructor. A deck can still start out empty until cards are passed to it. That should be handled in client code, especially if someone doesn't want a 52-card deck (or a certain game uses something different).

• As an alternative to using std::strings, consider enums for the ranks and suits:

  // make sure the first rank is explicitly set at 1
  // this will make the last rank equal 13
  enum Rank { Ace=1, Two, /* ... */, Queen, King };
  

  enum Suit { Spades, Hearts, Diamonds, Clubs };
  

  With this, you'll no longer need the arrays nor the max constants. Your m_rank and m_suit members should also be one of these enum type (Rank and Suit respectively). The enums and the Card class declaration should all then be contained in a namespace.

  Within your card class, you'll need functions to convert a given enum value to a char value for displaying. You could, for instance, do this with a switch statement, which will serve as a look-up table. This will also make it easier to maintain the ranks and suits.

  std::string getRank(Rank rank)
  {
      switch (rank)
      {
          case Ace: return "A";
          // ...
          default: throw std::logic_error("invalid rank");
      }
  }
  
  // same idea with suits
  

  Also note the default statement. If an invalid rank is passed in, an std::log_error exception will be thrown. Regardless of what works best for error-handling, there should be a default statement here to properly handle it.

• Rename Card2Str() to CardToString(). The former is just awkward wording. It should also be lowercase as it's a function and not a type.

• getOneCard() doesn't specify where in the deck the one card is drawn. Since it takes a card from the top of the deck, just call it draw() or deal(). On the other hand, it's void and has no parameters, so it's not returning a card. If this is supposed to just display a top card, then it's ineffective as it's still destroying the top card.

  Better yet, consider having a draw() and a top() (perhaps you just need to peek at the top card). The functionality also looks a bit unclear. Since you're using std::vector, a storage container, make use of its member functions. In this case, use back():

  draw():

  Card Deck::draw()
  {   
      Card cd = m_deck.back();
      m_deck.pop_back();
      return cd;
  }
  

  top():

  // return by const& as this Card should only be read-only
  // also make the function const as no data members are modified
  Card const& Deck::top() const
  {   
      return m_deck.back();
  }
  

  For both of these functions, be sure the caller is first checking for an empty deck.

  Side-note: You can use a different storage container if you, say, need to be able to insert into or draw from somewhere else in the deck besides the top or bottom. For that, you can use an std::list (doubly linked list). If you instead want to insert into or draw from the top and bottom, you can use an std::deque (double-ended queue). Otherwise, stay with what you have.

• Your deck is missing something: a shuffle function! You could utilize std::random_shuffle from the STL to do it nicely for you:

  void Deck::shuffle()
  {
      std::random_shuffle(m_deck.begin(), m_deck.end());
  }
  

  You should also check for an empty deck prior to shuffling.

Answer 2

I come from Java world, but here are just a few thoughts:

• generate_suit and generate_rank methods should probably not subtract 1 from MAXes and also adding zero at the end is unnecessary. E.g. rand() % (SUIT_MAX-1) is rand() % 3 and this could never be 3, so you will never have generated clubs ♣.

• Friending Card class by Deck class is not necessary. Just make get_suit and get_rank methods public and/or create a copy constructor of the Card class and/or rewrite getOneCard method like this:

  void Deck::getOneCard()
  {   
      std::cout << m_deck.back().Card2Str() << std::endl;
      m_deck.pop_back();        
  }
  

• Card::Card() constructor doesn't seem used now, so consider deleting it along with both generate* methods.

Answer 3

This is probably overkill for a poker program, but if you wanted to reuse your Deck class for card games other than poker you'll need to decouple it from your Card class.

In general, a Deck of cards is similar for all card games regardless of what type of cards are being used. Decks can be shuffled, cards can be drawn, etc.

Cards however can be poker cards, pinochle cards, memory game cards, UNO cards, or many more, not all have suits or ranks, though they all may have some way to print or display whatever values they do contain.

With that in mind, if you plan on making other card games besides poker, consider making an ICard abstract interface with some type of pure virtual display or print method, and having your Deck class depend on the interface instead of the Card class directly. Then you'll be able to perform Deck actions on any type of card for any type of card game you can dream up in the future. It will also mean that Deck should not be friends with any concrete Card class.

This added complexity though is not necessary if you only plan to make a poker game.

Answer 4

The Card constructor is unnecessarily complicated:

explicit Card(const int &suit, const int &rank)

Why not just take ints?

explicit Card(int suit, int rank)

---

There's not much that can be done with a Card, except to convert it into a string. What about comparing with another card for equality? How about comparing with another card by rank? That's impossible unless you stringify it and parse the string. The fact that Deck needs to be a friend is a red flag.

---

The Card class has a Card2Str() method. That's awkward, since the "Card" part of the name is redundant. In contrast, Deck has a print_Deck() method. Why the inconsistency in capitalization and in functionality? Neither method is idiomatic C++, which would provide an operator<< for std::ostream&. The ostream approach is also more flexible, since it is not constrained to write its output to std::cout. (You could write to a std::stringstream, for example, to obtain a string.)

---

Deck::getOneCard() is not useful, since it returns void and prints its result to std::cout instead. You want to return a Card, or more properly, a const Card& — and let the caller decide what to do with the card. The name is inappropriate, since "get…" has a connotation of having no side effect; drawCard() would be better. And if you can draw a card from the deck, there had better be a way to put it back at the bottom of the deck.