Random is not so Random

[z174]

I've tried my best, but I just cannot seem to find the problem with this code. What it is supposed to happen is:

Player 1 draws a card
Player 2 draws a card

(So neither of these cards would be available to play again.)

Unfortunately, they each draw a card and what happens is- they then draw the same suit (therefore, same numbers) repeatedly. I do not know where/how to fix this.

Please someone point me in the correct direction, it would be extremely appreciated!

Extra Information: The game being played is called 'War'. So the highest card wins.

#include <iostream>
#include <iomanip>
#include <ctime>
#include <cstdlib>
#include <algorithm>

using namespace std;


class Card
{
	public:
		Card();
		void displayCard();
		
		int getFace();
		char getSuit();
		
		void setCard( int, char);
		
	private:
		int cardVal;
		char suit[1];
};



class DeckOfCards
{
public:
  DeckOfCards();
  
  Card draw();
  void shuffle();
  bool isEmpty();

private:
  static const int MAX_CARDS = 52;       
  static const int NUM_SUITS = 4;       
  static const int CARDS_PER_SUIT = 13;  

  Card deck[MAX_CARDS];     
  int topCard;              

};


int main()
{
srand(time(0));


DeckOfCards deck;

Card card1;
Card card2;


int scoreOne = 0, scoreTwo = 0;
 deck.shuffle();


while (! deck.isEmpty())
{
card1 = deck.draw();
card2 = deck.draw();
	
cout << "Player 1: "; card1.displayCard();
cout << "\t\tPlayer 2: "; card2.displayCard(); 


if ( card1.getFace() > card2.getFace() )
{
	cout << endl << endl << "Player 1 wins the hand.";
	scoreOne += 2;
}
else if (card1.getFace() == card2.getFace())
{
	cout << endl << endl << "The hand is a draw!";
	scoreOne += 1;
	scoreTwo += 1;
}
else
{
	cout << endl << endl << "Player 2 wins the hand.";
	scoreTwo += 2;
}

cout << endl << "---------------------------------------------------" << endl << endl;
}


cout << "Player 1 score: " << scoreOne << endl;
cout << "Player 2 Score: " << scoreTwo << endl << endl;

if (scoreOne > scoreTwo)
{
	cout << "The winner is Player 1.";
}
else if (scoreOne == scoreTwo)
{
	cout << "The game is a draw!";
}
else
{
	cout << "The winner is Player 2.";
}
return 0;
}


Card::Card()
{
int randNum = rand() % 13 + 1;
if( randNum <= 13 )
  cardVal = randNum;

int randNum2 = rand() % 4 + 1;
if (randNum2 == 1)
{
	suit[1] = {'C'};
}
else if (randNum2 == 2)
{
	suit[1] = {'D'};
}
else if (randNum2 == 3)
{
	suit[1] = {'H'};
}
else if (randNum2 == 4)
{
	suit[1] = {'S'};
}
}


void Card::setCard( int newFace, char newSuit )
{
	cardVal = newFace;
	suit[1] = newSuit;
}


int Card::getFace()
{
	return cardVal;
}


char Card::getSuit()
{
	return suit[1];
}


void Card::displayCard()
{
	if (cardVal == 1)
	{
		cout << "Ace of ";
	}
	else if (cardVal == 11)
	{
		cout << "Jack of ";
	}
	else if (cardVal == 12)
	{
		cout << "Queen of ";
	}
	else if (cardVal == 13)
	{
		cout << "King of ";
	}
	else 
	{
		cout << "" << cardVal << " of ";
	}

if (suit[1] == 'C')
cout << "Clubs";
else if (suit[1] == 'D')
cout << "Diamonds";
else if (suit[1] == 'H')
cout << "Hearts";
else if (suit[1] == 'S')
cout << "Spades";
}

DeckOfCards::DeckOfCards()
{
char suitVals[NUM_SUITS] = { 'C', 'D', 'H', 'S' };
int cardSub = 0; 

for( int suitSub = 0; suitSub < NUM_SUITS; suitSub++ )
  {
  for( int faceVal = 1; faceVal <= CARDS_PER_SUIT; faceVal++ )
    {
    deck[ cardSub ].setCard( faceVal, suitVals[suitSub] );
    cardSub++;
	}
  }
topCard = -1;
}


Card DeckOfCards::draw()
{
topCard++;
return deck[topCard];
}


void DeckOfCards::shuffle()
{
random_shuffle(deck, deck+MAX_CARDS);
}


bool DeckOfCards::isEmpty()
{
return topCard + 1 == MAX_CARDS;
}

[SamuelAdams]

So you can either random shuffle the deck first or random pick the cards as you go.
If you shuffle the deck first then you only have to know which is next.
If you random pick the cards then you have to keep up with which are used, and pick another if it's used, so when there is only 5 cards left in the deck, it's going to be doing a lot of retries.

Either way you need a way to keep up with the order or if it's used, I think the best way is to use an array and pre-shuffle.

[JLBorges]

#include <iostream>
#include <string>
#include <cstdlib>
#include <iterator>
#include <numeric>
#include <algorithm>
#include <random>
#include <ctime>

struct card
{
    static const int NUM_SUITS = 4;
    static const int CARDS_PER_SUIT = 13;
    static const int NUM_CARDS = NUM_SUITS * CARDS_PER_SUIT ;

    // http://en.cppreference.com/w/cpp/numeric/math/abs
    card( int n = 0 ) : number( std::abs(n) % NUM_CARDS ) {}

    int number ; // 0 - 51

    std::string suit() const
    {
        static const std::string suit_str[CARDS_PER_SUIT] = { "Clubs", "Diamonds", "Hearts", "Spades" } ;
        return suit_str[number/CARDS_PER_SUIT] ;
    }

    int value() const { return number%CARDS_PER_SUIT ; } // 0 - 12

    std::string str() const
    {
        static const std::string value_str[CARDS_PER_SUIT] = { "Ace", "Two", "Three", "Four", "Five",
                                                               "Six", "Seven", "Eight", "Nine",
                                                               "Ten", "Jack", "Queen", "King" } ;
        return value_str[ value() ] + " of " + suit() ;
    }
};

struct deck_of_cards
{
    // http://en.cppreference.com/w/cpp/algorithm/iota
    // http://en.cppreference.com/w/cpp/iterator/begin
    deck_of_cards() { std::iota( std::begin(cards), std::end(cards), 0 ) ; }

    void shuffle()
    {
        // http://en.cppreference.com/w/cpp/header/random
        static std::mt19937 rng( std::time(nullptr) ) ;

        // http://en.cppreference.com/w/cpp/algorithm/random_shuffle
        std::shuffle( std::begin(cards), std::end(cards), rng ) ;
    }

    bool empty() const { return next == card::NUM_CARDS ; }

    card draw()
    {
        if( empty() ) next = 0 ;
        return cards[ next++ ] ;
    }

    card cards[card::NUM_CARDS] ;
    std::size_t next = 0 ;
};


int main()
{
    deck_of_cards deck ;
    deck.shuffle() ;

    int score_1 = 0 ;
    int score_2 = 0 ;

    while( !deck.empty() )
    {
        const card c1 = deck.draw() ;
        std::cout << "player 1 drew " << c1.str() << '\n' ;

        const card c2 = deck.draw() ;
        std::cout << "player 2 drew " << c2.str() << '\n' ;

        if( c1.suit() == c2.suit() )
        {
            std::cout << "The hand is a draw!\n\n" ;
            ++score_1 ;
            ++score_2 ;
        }
        else if( c1.suit() < c2.suit() )
        {
            std::cout << "Player 2 wins the hand.\n\n" ;
            score_2 += 2 ;
        }
        else
        {
            std::cout << "Player 1 wins the hand.\n\n" ;
            score_1 += 2 ;
        }
    }

    std::cout << "\n----------------------------------\n"
              << "Player 1 score: " << score_1 << '\n'
              << "Player 2 Score: " << score_2 << "\n\n";
}

http://coliru.stacked-crooked.com/a/e7c098ac66b4bd4f

[z174]

Thank you both for answering, but could you simplify it? I honestly have no idea what I'm doing. The code I presented took me 8 hours of struggling to come up with.

And unfortunately I have no idea how to read your code, because I've never seen some of those structures before.

The whole deck is randomly shuffled in the void DeckOfCards::shuffle() - which is then called into my main code.


This is what I have to do: (I've done one of these steps incorrectly.)

Set the seed value that is used by the random number generator by using the srand() function. Pass the value time(NULL) or time(0) to the srand function.

Create a DeckOfCards object that will used for the game.

Create two Card objects. These will be used to hold the cards that are drawn by the two people playing the game.

Create and initialize two integers to hold the game scores for the two players.

Shuffle the deck of cards by calling the shuffle method for the DeckOfCards object that was created earlier.

Write a loop that will execute as long as the deck of cards is not empty. Inside of the loop:

Player 1 should draw a card

Player 2 should draw a card

Display the two cards that were drawn

If the face values of the two cards are equal, awarded one point to both players and declare the "war" a draw. If player 1's card has the larger face value, award player 1 2 points and declare that player 1 won the "war". If player 2's card has the larger face value, award player 2 2 and declare that player 2 won the "war".

Once the deck of cards is empty, display the scores for both players and declare a winner. If the scores are equal, declare the battle a "draw".

[ne555]

You did good (except for the hideous indentation)


Your problem is with char suit[1]; on the `Card' class
You later access it like suit[1], which is out of bounds, as the array only have one element the only valid index was 0
I should be suit[0]

however, ¿why is it an array? ¿why don't simply char suit;?

[z174]

@ne555, Oh my goodness! Thank you so much. That fixed my problem. :D

But that is a good question. I'm not entirely sure why I used an array. Perhaps it was my inexperience leading me to believe only arrays could hold characters, which is silly because char literally means hold a character.

Anyway, thank you everyone for the help! <3