Pulling random cards from deck

[omegapatten]

The problem I am facing is that i have a sorted deck made and I am pulling cards out of it at random. As I pull cards i then store them and mark the card pulled as NULL and make its value 0, this then makes it possible for me to know which cards have already been taken out of the deck.

Well the problem is once i have pulled the card out and i go to pull another random one it will still pull cards i have already pulled. I have an "if" statement that should make this impossible but it still seems to happen.

I greatly appreciate any help that can be given as i am a beginner to c++.

Thank you very much beforehand.

source code
This code creates a sorted deck and has the function "pullRandomCard" which is the one giving me problems

#include <iostream>
#include <cstdlib>
#include <ctime>

using namespace std;

struct card
{
	int value;
	char suite;
};

card Deck [52];

void sortedDeck()
{
	int i = 1;
	int valueCounter = 1; 
	

	while (i <= 13)
	{
		Deck[i].suite = 'c';
		Deck[i].value = valueCounter;
		valueCounter++;
		i++;
	}
	valueCounter = 1;
	while (i <= 26)
	{		
		Deck[i].suite = 'd';
		Deck[i].value = valueCounter;
		valueCounter++;
		i++;
	}
	valueCounter = 1;
	while (i <= 39)
	{
		Deck[i].suite = 'h';
		Deck[i].value = valueCounter;
		valueCounter++;
		i++;
	}
	valueCounter = 1;
	while (i <= 52)
	{
		Deck[i].suite = 's';
		Deck[i].value = valueCounter;
		valueCounter++;
		i++;
	}
	
}

int temp;
card cardPulled;
void pullRandomCard()
{	
	srand ((unsigned)time(0));
	

	while(true)
	{
		temp = rand() % 52+1;
		if (Deck[temp].value != 0)
		{
			cardPulled.suite = Deck[temp].suite;
			cardPulled.value = Deck[temp].value;
			Deck[temp].suite = NULL;
			Deck[temp].value = 0;
			break;
		}
	}
}

[mcleano]

This can be significantly shortened to:

void sortedDeck() {
	int i = 1; // misses first element
	int valueCounter = 1; // not really needed?
   
        for(i; i<52; i++) {
             if(i<=13) {
                     Deck[i].suite = 'c';
                     Deck[i].value = i;
             }
             else if (i<=26)
       
             // ...
        }
}

Also you're not accessing the very first object: Deck[0].

Hmm I can't quite see why this isn't working although I feel it's just staring me in the face... I personally would create a vector and add all the card numbers you have pulled to the vector. Then, cycle through that vector everytime you come to pulling a card.

[omegapatten]

Problem Solved.
Thanks for the reply.

I solved the problem somehow. I honestly can say what I did specifically but it works fine now.

Thanks again.

[Athar]

There are a number of problems with the code, most notably that you have off-by-one index accesses. Array indices start at 0, not at 1.
Second, you should have a look at for loops, which are used whenever you need a loop with a counter.
Next, you have temp and cardPulled as global variables, but that isn't right. The card that is drawn from the deck should be returned by the function - not stored in some global variable. temp is used inside the pullRandomCard(), therefore it should be declared there. Something like randVal would be a better name anyway.

As for setting a card to NULL... you should note that NULL is just a macro define for 0. NULL should only be used to indicate that you mean a null pointer. Since you have no pointers here, that is not applicable.
Instead of doing that, you should actually remove the card from the array, which requires you to keep track of the current number of cards in the deck. At least in theory, because in practice you should use std::vector, which works like a dynamic array - you can add and remove elements at random without having to know the max number of elements in advance.

Last, don't include the srand call in pullRandomCard - whenever you initialize the pseudo-random number generator with the same value (which happens here when you call the function more than once per second), it will generate the same sequence of random numbers. Move the srand call to the beginning of main() - it should be called just once.

Oh, and a better name for sortedDeck would be either sortDeck - but since it doesn't really sort the deck, perhaps it should be called setupDeck or similar. It is also common practice to let variable and functions name start with a lowercase letter (i.e. deck instead of Deck).

So in my eyes, a proper implementation would be the following (with the exception that there should be an additional class "Deck" that manages the cards):

#include <iostream>
#include <vector>
using namespace std;

struct Card
{
  int value;
  char suit;

  Card() {}
  Card(int value,char suit) : value(value), suit(suit) {}
};

vector<Card> deck;

void setupDeck()
{
  deck.clear();
  static const char[]={'c','d','h','s'};
  for (int suit=0;suit<4;suit++)for (int val=1;val<=13;val++)deck.push_back(Card(val,suit));
}

Card drawRandomCard()
{ //you should probably handle the case when the deck is empty
  int randIndex=rand()%deck.size();
  Card drawnCard=deck[randIndex];
  deck.erase(deck.begin()+randIndex);
  return drawnCard;
}

[omegapatten]

Awsome! Thank you for all the great help!

I'll try my best to implement all these tips in future programs and in this one.

[firedraco]

You could also just use std::random_shuffle() and just keep popping the front or back.

[Duthomhas]

firedraco +1