Card Deck class for a Poker game

I have recently finished creating my own Deck class for my Poker game. It works the way I want it to, but I would like to know if I can make it better and/or more efficient.

Deck.h

#ifndef DECK_H
#define DECK_H

#include "Card.h"

#include 
#include 
#include 
using namespace std;

class Deck
{
private:
static const string RANKS[13], SUITS[4];

int size;
Card cards[52];

void build();

public:
Deck();
~Deck();

void shuffle();
Card drawCard();
int getDeckSize() const {return size;}
friend ostream& operator<<(ostream&, const Deck&);
};

#endif

Deck.cpp

#include "Deck.h"

const string Deck::RANKS[13] = {"A","2","3","4","5","6","7","8","9","10","J","Q","K"};
const string Deck::SUITS[4] = {"H","D","C","S"};

Deck::Deck() : size(0) {build();}

Deck::~Deck() {}

void Deck::build()
{
    srand((unsigned int)time(NULL));

    string card;
    bool isSameCard;

    while (size = 0; i--)
        cout << *((aDeck.cards)+i) << endl;

    return out;
}

Shuffling a Deck object only requires a call to shuffle(), so the object does not need to be destroyed for that. The shuffling algorithm works, and I tested this by making a separate check to make sure the deck never contains duplicates.

The driver for the class is in my Poker game itself, but I have tested each function sufficiently. I have both displayed the Deck and its size, and have drawn some cards, a few times each run.

I'm paying particular attention to this class because I want to be able to reuse it for future programs. That is one of my biggest interests in this post.

====== REVISED ======

Deck.h

```
#ifndef DECK_H
#define DECK_H

#include "Card.h"

#include
#include
#include
using std::ostream;
using std::string;
using std::vector;

class Deck
{
private:
static const int MAX_SIZE = 52;
static const string RANKS[13];
static const string SUITS[4];

int size;
vector cards;

public:
Deck();
~Deck();
Card drawCard();
void shuffle();
int getDeckSize

Never use using namespace X; in a header file. If I have to include your header you are polluting the global namespace for me and that can cause all sorts of problems in the code. As this is a potential source for bugs I will never use your header file (until you fix it).

using namespace std;

Even doing it in a normal *.cpp file is bad as it can cause problems for you. So it is better to fully qualify stuff std::string (the extra five letters is not so bad). If you must save those five letters you can bring specific objects into the current scope with using std::cout;.

Read all about it here: Why is 'using namespace std;' considered a bad practice in C++?

You are not using anything specific from iostream. All you seem to be using is the std::ostream but it has its own header file so just include that:

#include    // rather than 

You re not using time in the header file. So don;t include the in the header file. Only include the header files that you must include to get it to compile. In this case I would move ctime into the Deck.cpp file.

#include   // Move to Deck.cpp

Please one variable per line:

static const string RANKS[13], SUITS[4];

It makes the code easier to read. There is also one corner case (not hit here) that can hit you so best just not to do this.

If your destructor is not doing anything. Then don't write one:

Deck::~Deck() {}

Only use srand() once in an application.

void Deck::build()
{
    srand((unsigned int)time(NULL));

You are not doing yourself any favors with randomness by doing it more than once in an application. By putting it one place (probably just after main()) you know that you will not accidentally get two different pieces of code doing srand() and when debugging is being done you will want to make sure that you use the same sequence so putting srand() where it is easy to find and turn off can be useful.

Your build card could take a long time build the deck (if you get unlucky). An easier way is to put all the cards into the deck then shuffle it.

Good attempt at shuffle. But technically not correct (OK. for the people that complain about the last sentence I am merely quoting (from memory (so in spirit)) Knuth).

To do it correctly.

• Create an empty shuffled deck.

• You pick a card at random from your deck.

• Remove card from your deck.

• Put it on the top of the shuffled deck.

• If your deck is not empty repeat.

Or you can use the std::random_shuffle() algorithm. Which basically does the above algorithm (in place).

In your output operator. You pass the stream but don't use it (I assume that's just a type). Also I would prefer to use "\n" rather than std::endl. std::endl prints a newline then flushes the output. Probably not a performance problem here but in general unless you want to flush the stream prefer "\n".

cout << *((aDeck.cards)+i) << endl;  // for some reason you are
                                     // using std::cout

// Use `out` the stream passed in.

This is correct.

out << *((aDeck.cards)+i) << "\n";

But I think this is easier to read:

out << aDeck.cards[i] << "\n";

I would take this a step further and use a standard algorithm:

std::copy(std::begin(aDeck.cards), std::end(aDeck.cards),
          std::ostream_iterator(out, "\n"));

// Note: If you are using C++03 std::begin() and std::end() are
//       easily replaced with array index and address operations.

==========

Updated Code

static const integers can be defined and initialized in the class file:

// Deck.h
static const int MAX_SIZE = 52;

I would not try and do manual memory management:

Card *cards;  // New'ed on constructor and delete'd in destructor.

This is slightly more complex. Owned RAW pointers (this is one) means you need to implement the rule of three. This means you need to implement (or disable) the copy constructor and assignment operator. Otherwise you open yourself up to problems with double delete.

Note: An "Owned RAW pointer" is a pointer that you are responsible for deleting (this is to distinguish it from pointers that you are not responsible for deleting). Anything you create is usually owned. In most situations in C++ we use either smart pointers or containers to actually own the pointer so that our code can be concerned with business logic.

But a better solution is to use an existing container object:

std::vector  cards;

The principle of separation of Concerns means you should use the std::vector as the Deck is really an object that deals with business logic it should not also be handling resource management.

The shuffle is better:

void Deck::shuffle()
{
    Card *temp = new Card[MAX_SIZE];
    int counter = MAX_SIZE;

        for (int i = 0; i 
                     //         for `temp`. It will handle the memory
                     //         management correctly.

}

StackExchange Code Review Q#22805, answer score: 15