I am trying to generate a deck of cards using C++. I have already written all of the code, but there is a problem that I can't seem to figure out:
Deck::Deck(){
Card card;
bool match = false;
for (int i=0;i<47;i++){
do{
card.setCard();
match = cardInDeck(card, i);
}while(match == true);
match = false;
cards[i] = card;
}
numDrawn = 0;
}
In my constructor for the Deck class, I have a for() loop which generates all 52 cards and makes sure that the deck contains no matching cards. At least it should. The thing is, I can't make the loop iterate more than 47 times and still have it work. Any number over 47 causes the console screen to be empty upon run time, except for the blinking cursor. I am not quite sure what it is about numbers greater than 47 that cause it to stop working. I have tested it extensively and every number between 0 and 48 works.
Maybe I have some tiny error somewhere else in my code that I'm just not seeing. I don't really know. But I would really appreciate any help that I can get.
Here is my full code:
#include<iostream>
#include<stdlib.h>
using namespace std;
void run();
class Card{
private:
char suit;
int value;
public:
Card();
void setCard();
void getCard();
int getValue();
int getSuit();
};
class Deck{
private:
Card cards[52];
int numDrawn;
public:
Deck();
void shuffle();
void draw();
bool cardInDeck(Card card, int index);
};
int main(){
run();
}
Card::Card(){
srand(time(NULL));
value = rand() % 12 + 1;
suit = rand() % 4 + 1;
}
void Card::setCard(){
value = rand() % 12 + 1;
suit = rand() % 4 + 1;
}
void Card::getCard(){
cout<<" ----"<<endl<<"| |"<<endl<<"| ";
if (value == 1) cout<<'A';
else if (value == 10) cout<<'J';
else if (value == 11) cout<<'Q';
else if (value == 12) cout<<'K';
else cout<<value;
if (suit == 1) cout<<(char)3;
else if (suit == 2) cout<<(char)4;
else if (suit == 3) cout<<(char)5;
else cout<<(char)6;
cout<<" |"<<endl<<"| |"<<endl<<" ----"<<endl;
}
int Card::getSuit(){
return suit;
}
int Card::getValue(){
return value;
}
bool Deck::cardInDeck(Card card, int index){
bool match;
for(int i=0;i<=index;i++){
if((card.getValue() == cards[i].getValue()) && (card.getSuit() == cards[i].getSuit())){
match = true;
break;
}
else match = false;
}
return match;
}
Deck::Deck(){
Card card;
bool match = false;
for (int i=0;i<47;i++){
do{
card.setCard();
match = cardInDeck(card, i);
}while(match == true);
match = false;
cards[i] = card;
}
numDrawn = 0;
}
void Deck::shuffle(){
Card card;
bool match = false;
for (int i=0;i<52;i++){
do{
card.setCard();
match = cardInDeck(card, i);
}while(match == true);
match = false;
cards[i] = card;
}
numDrawn = 0;
}
void Deck::draw(){
cards[numDrawn].getCard();
numDrawn++;
}
void run(){
Deck cards;
char choice;
int cardsDrawn = 0;
cout<<"Enter 's' to shuffle the deck, 'd' to draw a card, or 'x' to exit: ";
do{
cin>>choice;
switch(choice){
case 'X':
case 'x':break;
case 'S':
case 's':cards.shuffle();
cout<<"Deck shuffled."<<endl;
cardsDrawn = 0;
break;
case 'D':
case 'd':if (cardsDrawn == 52){
cout<<"Out of cards. Deck reshuffled."<<endl;
cards.shuffle();
cardsDrawn = 0;
break;
}
else{
cards.draw();
cardsDrawn++;
break;
}
default: cout<<"Invalid entry.\a Enter a valid option('s','d','x'): ";
}
}while((choice != 'x') && (choice != 'X'));
}
There are 13 values in a 52 cards deck
Card::Card(){
srand(time(NULL));
value = rand() % 13 + 1;
suit = rand() % 4 + 1;
}
void Card::setCard(){
value = rand() % 13 + 1;
suit = rand() % 4 + 1;
}
12 * 4 -> 48
13 * 4 -> 52
Your original code with 12 values can only produce 48 different cards, this is why you get an infinite loop when you try to generate 52.
Edited :
By the way, you should follow Eric's and Hans Passant's (see comments on your question) advice. The way you do the shuffling is the wrong way to do it in the sense that there exists a much simpler / more natural / cleaner way. See below,
/**
* Forward counting implementation of Fisher-Yates / Knuth shuffle.
* see https://en.wikipedia.org/wiki/Fisher%E2%80%93Yates_shuffle
*/
template< typename A >
void shuffle ( A& a, int i, const int j ) {
// one item left => no need to shuffle
const int _j = j - 1;
for ( ; i < _j ; ++i ) {
// pick item uniformly at random to put at ith position
// once moved the item will stay in place
const int k = i + rand() % ( j - i );
// swap
auto tmp = a[i];
a[i] = a[k];
a[k] = tmp;
}
}
then you would have to generate all the 52 different cards once, like this
Card::Card( const int value, const int suit ) {
this->value = value;
this->suit = suit;
}
// we do not need this anymore
// void Card::setCard(){
// value = rand() % 13 + 1;
// suit = rand() % 4 + 1;
// }
Card cards[52];
int i = 0;
for ( int suit = 1 ; suit <= 4 ; ++suit ) {
for ( int value = 1 ; value <= 13 ; ++value ) {
cards[i] = Card( value, suit );
++i;
}
}
and finally shuffle the deck
shuffle( cards, 0, 52 );
More references on this common issue : http://bost.ocks.org/mike/shuffle and http://blog.codinghorror.com/the-danger-of-naivete.
Also please consider (as drescherjm sugested in his comment) to put the call to srand outside of this class. The call to srand resets the seed for the rand function and should in a very basic scheme only be called once at the very beginning of your main function. In your case, without a call to setCard() for each card you have, you might end up with 52 times the same card even though they are generated randomly ( see http://en.wikipedia.org/wiki/Pseudorandomness ).
I you have time you should look at the C++ random standard library header which provides way more the C rand lib. There even is a shuffle method in <algorithm>!