Duel-type card game in C

Question

This is similar to the game War, but with notable differences. For this program, it is the player versus the computer (so each with half of the deck). They draw the top card of their deck and compare. Whoever has the higher value card wins both cards and put both cards into the bottom of their decks (first the card the winner chose then the card the loser chose). Ranks and suits do matter when comparing. Order of rank and suits from highest to lowest:

• Ranks: King, Queen, Jack, 10, 9, 8, 7, 6, 5, 4, 3, 2, Ace
• Suits: Hearts, Diamonds, Clubs, Spades

The winner of the entire match, (or Duel as I call it in the program), is whoever ends up with the entire deck in their possession. Since the games generally get to several hundred moves, the game is automated.

/*
Card game in which the two players each have half of the deck.
Taking the top card of their deck, they compare the cards, seeing who has the
higher ranked card (compare rank then suit). Winner takes the two cards. To win
the game, the player must have the entire deck in his possession.
For this game, it will be the user versus the computer.
Based on the children's game, War
*/

#include <stdio.h>
#include <string.h>
#include <ctype.h>
#include <time.h>
#include <stdlib.h>
#include <unistd.h>

#define FALSE 0
#define TRUE 1

// Define suits, ranks, and respective names
enum Suit { Spades, Clubs, Diamonds, Hearts};
enum Rank { Ace, Two, Three, Four, Five, Six, Seven, Eight, Nine, Ten, Jack, Queen, King};
char * SuitNames[] = {"Spades", "Clubs", "Diamonds", "Hearts"};
char * RankNames[] = {"Ace", "Two", "Three", "Four", "Five", "Six", "Seven", "Eight", "Nine", "Ten", "Jack", "Queen", "King"};

// struct for a card
typedef struct CardStructure
{
    enum Suit suit;
    enum Rank rank;
} Card;

// Method prototyping
void playMove(Card * playerHand[], Card * compHand[]);
void dealCards(Card * playerHand[], Card * compHand[], Card * deck[]);
int compareCards(Card * playerCard, Card * compCard);
int checkWin(Card * playerHand[], Card * compHand[]);


int main()
{
    Card * playerHand[52];
    Card * compHand[52];
    Card * deck[52];
    char playAgain;

    puts("Welcome to War! You literally duel the computer by comparing\n"            // Basic greeting and introduction
           "your top card with the computer's top card. Whoever has the highest\n"
           "value card takes the two cards and puts them onto the bottom of their\n"
           "deck. The winner of the game is determined by whoever ends with the\n"
           "entire deck in their possession.\n"
           "From highest to lowest:\n"
           "\tRanks: A K Q J 10 9 8 7 6 5 4 3 2\n"
           "\tSuits: Hearts, Diamonds, Clubs, Spades\n"
           "The game will start shortly.\n");
    sleep(25);                                   // Give user time to read the greeting

    // Main game loop
    do
    {
        dealCards(playerHand, compHand, deck);   // Give the player and computer half the deck
        int moves = 1;                           // Keeps track of number of moves

        while (1)
        {
            printf("Move: %d\n", moves);
            playMove(playerHand, compHand);                 // Player and computer draw their top card and compare, finish turn
            int result = checkWin(playerHand, compHand);
            if (result == 1)
            {
                puts("The player has won the duel!");
                break;
            }
            else if (result == 2)
            {
                puts("The computer has won the duel!");
                break;
            }

            moves++;
        }

        printf("\n\nWould you like to play again? Enter Y for yes, anything else for no: ");  // Prompt to restart game
        scanf(" %c", &playAgain);
        if (toupper(playAgain) != 'Y')
        {
            for (int index = 0; index < 52; index++)
            {
                free(deck[index]);          // Cards were malloced in dealCards(), time to free them
            }
            break;
        }
    } while (1);
    return 0;
}


void dealCards(Card * playerHand[], Card * compHand[], Card * deck[])
{
    int cardsCreated = 0;
    int turn = 0;         // Keeps track of who is supposed to get the card, player = 0, computer = 1
    Card card;            // Card template
    srand(time(NULL));    // Randomize the seed

    while (cardsCreated < 52)
    {
        int cardFound = FALSE;
        card.rank = rand() % 13;
        card.suit = rand() % 4;

        for (int index = 0; index < cardsCreated; index++)
        {
            Card * deckCard = deck[index];  // Take a card from the deck...

            if (deckCard->rank == card.rank && deckCard->suit == card.suit) // ...and compare it to the newly made card
            {
                cardFound = TRUE;  // Card is a duplicate, exit loop and continue
                break;
            }

        }

        if (cardFound == FALSE)
        {
            if (turn == 0) {
                playerHand[cardsCreated/2] = ( Card *) malloc ( sizeof(Card)); // Malloc the card and give player the card
                playerHand[cardsCreated/2]->suit = card.suit;
                playerHand[cardsCreated/2]->rank = card.rank;
                deck[cardsCreated] = playerHand[cardsCreated/2];               // Add card to deck for comparison purposes
            }
            else if (turn == 1) {
                compHand[(cardsCreated-1)/2] = ( Card *) malloc ( sizeof(Card)); // Malloc the card and give computer the card
                compHand[(cardsCreated-1)/2]->suit = card.suit;
                compHand[(cardsCreated-1)/2]->rank = card.rank;
                deck[cardsCreated] = compHand[(cardsCreated-1)/2];               // Add card to deck for comparison purposes
            }

            turn = (turn == 0) ? 1 : 0;    // Switch turn from 0 -> 1 or 1 -> 0
            cardsCreated++;
        }
    }

    for (int index = 26; index < 52; index++)   // Set the non-existent cards to NULL
    {
        playerHand[index] = NULL;
        compHand[index] = NULL;
    }
}



void playMove(Card * playerHand[], Card * compHand[])
{
    Card * playerCard = playerHand[0];      // Get top cards and their information
    Card * compCard = compHand[0];
    int pSuit = playerCard->suit;
    int pRank = playerCard->rank;
    int cSuit = compCard->suit;
    int cRank = compCard->rank;
    int result = compareCards(playerCard, compCard);    // If player has the better card, returns 0, otherwise returns 1

    if (!result)
    {
        printf("The player won this turn.\n"
               "\tPlayer's card: %s of %s\n"
               "\tComputer's card: %s of %s\n", RankNames[pRank], SuitNames[pSuit], RankNames[cRank], SuitNames[cSuit]);
        for (int index = 1; index < 52; index++)
        {
            playerHand[index-1] = playerHand[index];    // Shifts every card forward (subtracts one from their index)
            compHand[index-1] = compHand[index];
        }

        int length = 0;
        for (int index = 0; index < 52; index++)        // Calculate how many cards in the player's hand with the top card discarded
        {
            if (playerHand[index] != NULL)
                length++;
        }
        playerHand[length] = playerCard;                // Place discarded top card to the bottom of the deck
        playerHand[length+1] = compCard;                // Place the won card to the bottom of the deck
    }

    else
    {
        printf("The computer won this turn.\n"
               "\tPlayer's card: %s of %s\n"
               "\tComputer's card: %s of %s\n", RankNames[pRank], SuitNames[pSuit], RankNames[cRank], SuitNames[cSuit]);
        for (int index = 1; index < 52; index++)
        {
            playerHand[index-1] = playerHand[index];    // Shifts every card forward (subtracts one from their index)
            compHand[index-1] = compHand[index];
        }

        int length = 0;
        for (int index = 0; index < 52; index++)        // Calculate how many cards in the computer's hand with the top card discarded
        {
            if (compHand[index] != NULL)
                length++;
        }
        compHand[length] = compCard;                    // Place discarded top card to the bottom of the deck
        compHand[length+1] = playerCard;                // Place the won card to the bottom of the deck
    }
}


int compareCards(Card * playerCard, Card * compCard)
{
    if (playerCard->rank > compCard->rank)      // Compares ranks
        return 0;
    else if (playerCard->rank < compCard->rank)
        return 1;
    else
    {
        if (playerCard->suit > compCard->suit)  // If ranks match, compare suits
            return 0;
        else
            return 1;
    }
}


int checkWin(Card * playerHand[], Card * compHand[])
{
    int playerLen = 0;
    int compLen = 0;

    for (int i = 0; i < 52; i++)        // Number of cards is determined by the number of non-NULL cards
    {
        if (playerHand[i] != NULL)
            playerLen++;
        if (compHand[i] != NULL)
            compLen++;
    }
    printf("Player deck size: %d\nComputer deck size: %d\n"
           "----------------------------------------------\n", playerLen, compLen);     // Output current size of player's and computer's deck

    if (playerLen == 52)    // Player has entire deck, player wins
        return 1;
    if (compLen == 52)      // Computer has entire deck, computer wins
        return 2;
    return -1;              // No one has entire deck, continue play
}

My question is that what can I do to improve the program, specifically around the areas of code logic and style (like the C equivalent of Python's PEP8)?

Answer 1

There's no major issue. Beautiful code. - That's a matter of taste, but you could define TRUE and FALSE in their logical terms:

#define TRUE  (1==1)
#define FALSE (!TRUE)

• moves could be an unsigned

---

dealCards

int turn = 0;

• turn should be simply renamed computerTurn (see below why)

• and since you defined TRUE/FALSE, use it here : int computerTurn = FALSE;.

• also, if (turn == 0) can be explicitly changed to if (!computerTurn)

• and else if (turn == 1) to else if (computerTurn) or even just else.
• turn = (turn == 0) ? 1 : 0; can be rewritten with the logical unary not: computerTurn = !computerTurn;.
• Instead of writing two times almost the same code...

playerHand[cardsCreated/2] = ( Card *) malloc ( sizeof(Card)); // Malloc the card a...
playerHand[cardsCreated/2]->suit = card.suit;
playerHand[cardsCreated/2]->rank = card.rank;

       ...make cards allocation and creation outside of the condition (see my example below).

• Also, you could omit to cast the result of malloc and use the variable name into sizeof instead of using the type.

---

Instead of your logic, you can try this:

• Make an array of 52 int :

int cards[52];
for (unsigned int i = 0; i < 52; i++) {
    cards[i] = i;
}

• Then, shuffle it (maybe with the Fisher–Yates shuffle).
• Now you just have to alternatively deal card from this array to player and computer (and after, as you do, set non-existent cards from both hands, to null).

for (unsigned int i = 0; i < 52; i++) {
    Card *current = malloc(sizeof(card));
    current->suit = cards[i] % sizeof(SuitNames)/sizeof(*SuitNames);
    current->rank = cards[i] % sizeof(RankNames)/sizeof(*RankNames);

    if (!computerTurn) playerHand[i/2] = current;
    else computerHand[i/2] = current;

    computerTurn = !computerTurn;
}

I see some advantages of this method:

• You can get rid of the variable deck
• The generation and dealing in constant in time, where with your method, if you are out of luck, filling both hands can take long time.

---

playMove

I didn't understand advantage of pSuit, pRank, cSuit and cRank against their arrowed counter-part.

result should be renamed computerWin it's more explicit.

A lot of duplicated code can be moved out of the if...else (before or after), but above all, a major optimization could be to keep a track of how many cards each hand have (playerCount and compCount).

• You don't have anymore to shift left every card in both deck
• You don't have to compute how many card each hand have
• You also have easily access to the current card (playerHand[playerCount-1])

In fact, maybe you can wrap the "hand" and the "count" into a struct:

typedef struct HandStructure
{
    Card *cards[52];
    size_t count;
} Hand;

---

compareCards

You can simplify branching a lot:

int compareCards(Card * playerCard, Card * compCard)
{
    if (playerCard->rank <= compCard->rank)      
        return (playerCard->rank < compCard->rank) || (playerCard->suit < compCard->suit)
    else return 0;
}

---

checkWin

You should return 0 instead of -1 if no one won, it allow if(checkWin(...)) in the main.

Dealing with the Hand structure which I talked about, this function became also shorter and simple:

int checkWin(Hand * player, Hand * computer)
{
    assert(player->count + computer->count == 52);

    if (!computer->count)    // Player has entire deck, player wins
        return 1;
    if (!player->count)      // Computer has entire deck, computer wins
        return 2;
    return 0;                // No one has entire deck, continue play
}

---

edit: Also, you should define both char* at the top as const.

End words

Also, as you can see, I introduced assertion in the last code, paradoxically to the length of this post, i found code pretty well writes. It's time, I think, to adopt stronger concepts and methods. Assertions are one of those things.

I hope not being too straight or rude, English isn't my primary language, so I miss some nuancies.

---

10 days later...

I wrote this few days ago, but didn't take time to add it before. Hope it will help.

Cards

Actually in your code, you compare cards with ranks, and then if equals, with suits.

So the lowest card is the Ace of Spikes and the greatest the King of Hearts. Also, the Ace of Spikes is lower than the Ace of Clubs which is lower than all others.

So, what's cards values?

• 1: Ace of Spikes

• 2: Ace of Clubs

• 3: Ace of Diamonds

  ...

• 51: King of Diamonds

• 52: King of Hearts

So, instead of dealing with suits and ranks, you can try to deal with values directly. The question is now: "How retrieve suits and ranks from a card's value?"

Pretty simple!

(with card values going from 0 to 51)

//ensure card value < (number of suits * number of ranks), then:
Suit = card value % number of suits
Rank = card value / number of ranks

Working example

Let's test it (pseudo-code):

array suits[4] = {"Spike", "Club", "Diamond", "Heart"}
array ranks[13] = {"Ace", "Two", ..., "Queen", "King"}

let ace_of_spikes = 0
let ace_of_diamonds = 2
let king_of_diamonds = 50
let king_of_hearts = 51

print suit[ace_of_spikes  % sizeof suits] -> "Spike"
print suit[ace_of_diamonds  % sizeof suits] -> "Diamond"

We simply divided memory usage by two, using only one integer instead of two per card.

In this same way, you also make a lot of things in your code easier (build of deck(s), comparison of card to get winner, ...).

About the modulus operator, even if it can be slow before trying to optimize it, you should ensure it's needed with benchmarks. (other related links: [1], [2], [3]).

Deck(s)

Just think two second about this game (in your program and in real life).

Each player have card from the same deck, right? The deck is just split into halves to players.

I got the intuition that we can work with this unique deck, and using two virtually splitted up views of it as players' sub-deck. After all, in real life, if you join the cards from both players, it's always the same deck.

Let's try to set that idea into practice!

• Virtually splits our deck at the middle. Let's call this position "separator"
• Each player take cards from this separator until their sides.
• When a player won a card, it come in his side of the separator and so became a part of his sub-deck.
• When a player reach the last card of his part of the deck, he continues taking the card right before the separator, i.e. his new top sub-deck. In real life, before picking this card, his part of the deck is shuffled. Let's assume that we can do the same, that will add a bit randomization.
• Finally, when he don't have card from his side of separator, he have lost.

Visual example

Legend:

▲ : The current card of each player

⋆ : The card that was just won

Turn 1:

#1 : 1 7 2 3 8|4 5 9 6 0 : #2
             ▲ ▲  

#1 got a 8, #2 got a 4 ➔ #1 won the 4

We move pointers to the next cards, and the separator to the right to give one card from #2 to #1 and.

Note: In fact, we swap the right-most card (the nearest of the separator) of his sub-deck with the card he have just won. Since here it's the same card, it's a no-op.

#1 : 1 7 2 3 8 4|5 9 6 0 : #2
           ▲   ⋆ ▲

Since 4 is already in the deck #1, we have noting to do.

---

Turn 2:

#1 : 1 7 2 3 8 4|5 9 6 0 : #2
           ▲     ▲

#1 got a 3, #2 got a 5 ➔ #2 won the 3

Let's move pointers to the next cards, and the separator to the left to give one card from #1 to #2 and.

#1 : 1 7 2 3 8|4 5 9 6 0 : #2
         ▲ ⋆       ▲

Here, 3 isn't is already in the deck #2, we have to swap it with the card nearest of the separator, on his side.

Note: We could, instead, have operate a circular left shift of card 3 8 4 to get 8 4 3 and so, keep a consistent order, but since we'll shuffling cards of a player when he have no more not already played card (i.e when we'll have to move the pointer to the card before the separator), it's not necessary.

#1 : 1 7 2 4 8|3 5 9 6 0 : #2
         ▲     ⋆   ▲

---

Turn 3:

#1 : 1 7 2 4 8|3 5 9 6 0 : #2
         ▲         ▲

#1 got a 2, #2 got a 9 ➔ #2 won the 2

We move pointers to the next cards, and the separator to the left to give one card from #1 to #2 and.

#1 : 1 7 2 4|8 3 5 9 6 0 : #2
       ▲ ⋆           ▲

Since, 2 isn't is in the deck #2, we swap it with the card at the right of the separator.

#1 : 1 7 8 4|2 3 5 9 6 0 : #2
       ▲     ⋆       ▲

---

Let's go faster with this logic, showing just results

Turn 4 :

#1 : 1 7 8 4 2|3 5 9 6 0 : #2
       ▲             ▲

#1 : 1 7 8 4 2 3|5 9 6 0 : #2
     ▲               ⋆ ▲

#1 : 1 7 8 4 2 6|5 9 3 0 : #2
     ▲         ⋆       ▲

---

Turn 5:

#1 : 1 7 8 4 2 6|5 9 3 0 : #2
     ▲                 ▲

Let's start with same as usual

#1 : 1 7 8 4 2 6 5|9 3 0 : #2
   ▲                   ⋆ ▲
   ?                     ?

#1 : 1 7 8 4 2 6 0|9 3 5 : #2
   ▲             ⋆       ▲
   ⤷                     ⤶

But here, both players have no more card to draw, let's shuffle their part of the deck, and move pointers before the separator.

#1 : 4 2 0 6 1 7 8|3 5 9 : #2
                 ▲ ▲      

And this, until a player doesn't have card anymore.

---

Final End words

As stated in comments, the player who have to top deck card is his sub-deck automatically win. So here, you also have two options: - Provide option to just print the winner, without computation of turns - Rethink some functions (playMove and checkWin in your case) to handle "draws" (same ranks).

If you want, I can try to come back in few days with a example of implementation.

Answer 2

You've already heard about efficient shuffling in Calak's excellent answer. I have just one thing to add to it: don't malloc each card independently. Card is two enums. Even if each of these are represented as an int, then Card is an 8-byte data structure (though it is possible your compiler uses only one byte for each enum value):

typedef struct CardStructure
{
    enum Suit suit;
    enum Rank rank;
} Card;

A pointer to such a structure is also 8 bytes (assuming you're on a 64-bit OS). So, the array of 52 pointers to Card takes up just as much space as an array of 52 Card objects. Calling malloc is relatively expensive, you need an extra indirection (pointer dereference) for each card read, and you end up with data (potentially) all over the heap, which is bad for cache usage. So even if Card were larger (32 bytes, 64 bytes) it would be beneficial to store them directly inside an array, and copy them around. An additional advantage is that you don't need to remember to free the cards.

I recommend malloc only for cases where you cannot store data on the stack: in a function that creates an object and returns it, or for a large array.

---

Now, considering that there are only 52 unique cards, we know it should be possible to store one card in one byte. One approach could use a bit field like this:

typedef struct CardStructure {
   unsigned int suit : 2; // allows any integer in [0,3]
   unsigned int rank : 4; // allows any integer in [0,15]
} Card;

It should be possible to still assign the enum Suit and enum Rank values to these bit fields. But I must say that this is an optimization that maybe goes a little too far, the original Card is easier and cleaner, and therefore the better choice.

---

Oh, I just realized that you are not actually using the enum constants anywhere in your code. You are just using their numeric values. You are better off not defining those enums in that case, and just use a char to store each value.

---

Bug: Your documentation says that the cards are ordered: "A K Q J 10 9 8 7 6 5 4 3 2", but your code logic says that A is the lowest card.

Answer 3

My question is that what can I do to improve the program, specifically around the areas of code logic and style ... ?

Use stdbool.h

Since C99 @Lundin, C has the _Bool type which has 2 values: 0 and 1. Use that rather than

// #define FALSE 0
// #define TRUE 1
// int cardFound = FALSE;
// cardFound = TRUE;  
// if (cardFound == FALSE)

use

#include <stdbool.h>
bool cardFound = false;
cardFound = true;  
if (!cardFound)

Flush output

stdout is commonly line buffered, but may be unbuffered or fully buffered. To insure out is seen before requesting input, (especially when that output does not end in a '\n') us fflush().

 printf("\n\nWould you like to play again? Enter Y for yes, anything else for no: ");
 fflush(stdout);
 scanf(" %c", &playAgain);

Format to presentation width

Code should be auto-formated so re-formating to a different preferred max width should be easy. Reviewing code that rolls far off the right of the screen reduced code review efficiency. OP's width is 143+. Suggest something in the 75-100 range.

char * RankNames[] = {"Ace", "Two", "Three", "Four", "Five", "Six", "Seven", "Eight", "Nine", "Ten", "Jack", "Queen", "King"};

printf("\n\nWould you like to play again? Enter Y for yes, anything else for no: ");  // Prompt to restart game
scanf(" %c", &playAgain);

           "----------------------------------------------\n", playerLen, compLen);     // Output current size of player's and computer's deck 

vs.

char * RankNames[] = {"Ace", "Two", "Three", "Four", "Five", "Six", "Seven", 
    "Eight", "Nine", "Ten", "Jack", "Queen", "King"};

// Prompt to restart game
printf("\n\n" // 
   "Would you like to play again? Enter Y for yes, anything else for no: ");
scanf(" %c", &playAgain);

    // Output current size of player's and computer's deck
    "----------------------------------------------\n", playerLen, compLen);

Allocate to the object and drop cast

Consider the below. The first obliges a check: Was the correct type used? The 2nd does not need that check. The unnecessary cast is WET. The 2nd is DRY, easier to code right, review and maintain.

// playerHand[cardsCreated/2] = ( Card *) malloc ( sizeof(Card));
playerHand[cardsCreated/2] = malloc (sizeof *playerHand);

Robust code would also detect allocation failures.

if (playerHand[cardsCreated/2] == NULL) {
  // call out of memory handler
}

Reduce rand() calls

2 calls to rand() is twice the time when one would do.

// card.rank = rand() % 13;
// card.suit = rand() % 4;
int r = rand();
card.rank = (r/4) % 13;
card.suit = r%4;

Remove else

Minor style issue: compareCards() looks like it is missing a return at the function end. (it is not though)

Alternate layout:

int compareCards(Card * playerCard, Card * compCard) {
  if (playerCard->rank > compCard->rank) { // Compares ranks
    return 0;
  }
  if (playerCard->rank < compCard->rank) {
    return 1;
  } 
  if (playerCard->suit > compCard->suit) { // As ranks match, compare suits
    return 0;
  }
  return 1;
}

Other simplification possible.

Employ const

When a function parameter points to unchanged data, make it const. This allows for wider code use, some optimizations and conveys better codes intent.

// int checkWin(Card * playerHand[], Card * compHand[]);
int checkWin(const Card * playerHand[], const Card * compHand[]);

Unused non-standard include

The only reason for the non-standard C <unistd.h> seems to be sleep(). Consider removal for greater portability.

Technical undefined behavior (UB)

toupper(ch) is valid for int values in the unsigned charrange and EOF. Else UB when ch < 0.

    scanf(" %c", &playAgain);
    // if (toupper(playAgain) != 'Y')
    if (toupper((unsigned char) playAgain) != 'Y')

Design - missing dealCards() companion

Rather than code in main() the free-ing for (int index = 0; index < 52; index++) free(deck[index]);, Consider a companion function to un-deal.