1
\$\begingroup\$

I have written this program to deal 7 cards to one player.

#include <iostream>
#include <string>
#include <cstdlib> //for rand and srand
#include <cstdio>
#include <vector>
using namespace std;

string suit[] = {"Diamonds", "Hearts", "Spades", "Clubs"};
string facevalue[] = {"Two", "Three", "Four", "Five", "Six", "Seven",         "Eight", "Nine", "Ten", "Jack", "Queen", "King", "Ace"};
int numberOfCardsDrawn = 0;
string drawnCards[52];

string drawCard () {
    string card;
    int cardvalue = rand()%13;
    int cardsuit = rand()%4;
    card += facevalue[cardvalue];
    card += " of ";
    card += suit[cardsuit];
    return card;
}


bool isMyCardAlreadyDrawn (string card) {
    for(int i = 0; i < 52; i++) {
        if(card.compare(drawnCards[i]) == 0) { 
            // if this is true, then both strings are the same
            return true;
        }
    } 
    return false; 
    // if the code reaches this point, then the card has not   been drawn yet
}

string getCard () {
    srand(time(0));
    string card = drawCard();
    while (isMyCardAlreadyDrawn(card) == true) { 
        // keep drawing until an    undrawn card is found
        card = drawCard(); // draw a new card
    }
    drawnCards[numberOfCardsDrawn] = card;
    numberOfCardsDrawn++;
    return card;
}

int main () {
    cout << "Your Cards:\n";
    vector<string> p0; //player 0's card
    const int DEAL_CARDS = 7; //number of cards we can deal to each player
    string choices[] = { "a", "b","c","d","e","f","g" };

    for (int i = 0; i < DEAL_CARDS; i++){
        string p0_getCard = getCard();
        cout <<" " << choices[i] << ")"<< p0_getCard << " ";
        p0.push_back(p0_getCard);
    }
}

However, I will eventually want to have a five-player game (dealing cards to four more vectors), and I feel that extending this code to accomplish that would result in excessive code duplication. How do I go about improving the code to support that?

\$\endgroup\$
10
  • 1
    \$\begingroup\$ Reasonable approach would be to have all cards in one array, and rand on it. Then you take next available card (circular approach). \$\endgroup\$
    – VladimirS
    Mar 31, 2016 at 14:07
  • 3
    \$\begingroup\$ std::deque models... a deck of card \$\endgroup\$ Mar 31, 2016 at 14:12
  • \$\begingroup\$ You should learn more about the stl. You may just make a std::vector and remove every drawn card. array would be possible, too. \$\endgroup\$
    – Mehno
    Mar 31, 2016 at 14:13
  • \$\begingroup\$ make an array with all the cards in, shuffle it, and then take the first N in order. \$\endgroup\$
    – Alexander Ekzhanov
    Mar 31, 2016 at 14:30
  • \$\begingroup\$ @RichardHodges And how do you think he can use internal realisation of std::deque for his purposes? \$\endgroup\$
    – LibertyPaul
    Mar 31, 2016 at 15:12

3 Answers 3

3
\$\begingroup\$

I see a number of things which may help you improve your code.

Don't abuse using namespace std

Especially in a very simple program like this, there's little reason to use that line. Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Don't reseed the random number generator more than once

The program currently calls srand at the top of each call to getCard(), but this is really neither necessary nor advisable. Instead, just call it once when the program begins and then continue to use rand() to get random numbers. Better yet, use the next suggestion.

Consider using a better random number generator

You are currently using

int cardsuit = rand()%4;

There are two problems with this approach. One is that the low order bits of the random number generator are not particularly random, so neither with cardsuit be. On my machine, there's a slight but measurable bias toward 0 with that. The second problem is that it's not thread safe because rand stores hidden state. A better solution, if your compiler and library supports it, would be to use the C++11 `std::uniform_int_distribution. It looks complex, but it's actually pretty easy to use.

Eliminate global variables where practical

Having routines dependent on global variables makes it that much more difficult to understand the logic and introduces many opportunities for error. Eliminating global variables where practical is always a good idea, whether programming for desktop machines or for embedded systems. For global variables such as drawnCards and `numberOfCardsDrawn, consider wrapping them in objects to more easily keep them synchronized.

Use object-oriented programming

The game is written much more in the procedural style of C rather than in the object-oriented style of C++. The cards and the deck could each be an object, with most of the procedures as functions of those objects. This would reduce coupling and make the program easier to understand.

Eliminate "magic numbers"

This code is littered with "magic numbers," that is, unnamed constants such as 4, 13, 52, etc. Generally it's better to avoid that and give such constants meaningful names. That way, if anything ever needs to be changed, you won't have to go hunting through the code for all instances of "52" and then trying to determine if this particular 52 means the total number of cards or some other constant that happens to have the same value.

Use const where practical

In your isMyCardAlreadyDrwan routine, the passed card is never altered, which is just as it should be. You should indicate that fact by declaring it like this:

bool isMyCardAlreadyDrawn (const std::string& card)
\$\endgroup\$
1
\$\begingroup\$

You should use a class (or structure) for describing a card

class Card{
    int suit;
    int value;

    static constexpr array<string, 4> suitNames = {...};
    static constexpr array<string, 13> valueNames = {...};

    Card(int suit, int value): suit(suit), value(value){}
    string toString() const;
    int value() const;
    int suit() const;
};

And emplace Card objects in container such as vector<Card>. You can initialize it with full deck, shuffle, and distribute among players:

vector<Card> deck;
for(int suit = 0; suit < 4; ++suit){
    for(int value = 0; value < 13; ++value){
        deck.push_back(Card(suit, value));
    }
}

shuffle(deck.begin(), deck.end(), default_random_engine());

for(int player = 0; player < playerCount; ++player){
    Card current = deck.back();
    players[player].addCard(current);
    deck.pop_back();
}
\$\endgroup\$
0
\$\begingroup\$

Better technique.

I would generate all the cards first. Then randomly shuffle the pack. Then you can draw from the pack until you have no cards.

class ShoeOfCards
{
     std::vector<Card>   pack;
     int                 currentTop;
     public:
         ShoeOfCards(int numberOfPacks)
             : currentTop(0)
         {
             pack.reserve(numberOfPacks * 52);

             for(int pack = 0; pack < numberOfPacks; ++pack)
             {
                 for(int suit = 0; suit < 4; ++suit)
                 {
                     for(int rank=0; rank < 13; ++rank)
                     {
                         pack.emplace_back(Suit(suit), Rank(rank));
                     }
                 }
             }
             std::random_shuffle(std::begin(pack), std::end(pack),
                                 getRandomNumberGenerator());
        }

        Card dealCard()
        {
            return pack[currentTop++];
        }
};
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.