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
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 <ostream> // rather than <iostream>
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 <ctime> // Move to Deck.cpp
Please one variable per line:
static const string RANKS, SUITS;
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:
Only use srand() once in an application.
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:
// Note: If you are using C++03 std::begin() and std::end() are
// easily replaced with array index and address operations.
static const integers can be defined and initialized in the class file:
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:
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:
Card *temp = new Card[MAX_SIZE];
int counter = MAX_SIZE;
for (int i = 0; i < MAX_SIZE; i++)
int j = rand() % MAX_SIZE; // Note: The size is getting smaller
// each time you remove a card.
// So you should use %(MAX_SIZE - i);
*cards = *temp; // Note: You have made a call to new.
// As a result you should call delete
// to avoid a memory leak.
// A better way to do this is:
// std::swap(cards, temp);
// delete  temp;
// Note 2: Note your code here merely swaps
// the first card from the two arrays.
// Note 3: You should probably use std::vector<Card>
// for `temp`. It will handle the memory
// management correctly.