patterncppModerate

Card and Deck classes

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

anddeckcardclasses

Problem

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 

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      /* srand, rand */
#include "card.h"
#include 

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 
#include 
#include 
#include "card.h"

using namespace std;

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

};

#endif

deck.cpp

```
#include
#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 D

Solution

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

-
` and time.h are both C libraries. Use the respective C++ libraries and . The rest are okay. You don't need to include

 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 enum

s 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.

Code Snippets

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

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

// 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 };

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

// same idea with suits

Context

StackExchange Code Review Q#41810, answer score: 12

Revisions (0)

No revisions yet.