4
\$\begingroup\$

I am new to C and wanted to get feedback on a small project I was working on. Coming from python, I quickly learned that a lot of things we take for granted like list class and all of its useful functions are not implemented. I wanted to see if I could make a card library to help me build multiple types of card games. I just created a demo Blackjack game for example (Yes Ace is only counted as 1 for the demo) to show how someone would use it. If possible, can you let me know what I have done well (if any), what I can improve on and any other information that is helpful?

below is card.h

//card.h
#ifndef CARD_H
#define CARD_H

typedef struct Card Card;
typedef struct Deck_node Deck_node;
typedef struct Deck Deck;

struct Card{
    int suit;
    int number;
    int face;
    char* symbol;
};

struct Deck_node{
    Deck_node* prev;
    Card* card;
    Deck_node* next;
};

struct Deck{
    Deck_node* head;
    Deck_node* tail;
    int size;
};



Card *card_create(int suit, int number, char *symbol, int symbol_length);

Deck_node* deck_node_add_next(Deck_node *deck_node, Deck_node *next); 

Deck_node *deck_node_create(Card *card); 

void deck_node_destroy(Deck_node *deck_node);

Deck *deck_create();

void deck_clear(Deck *deck);


void deck_append_node(Deck *deck, Deck_node *deck_node); 

int deck_random_card_index(Deck *deck);

void deck_node_switch(Deck_node *n1, Deck_node *n2); 

void deck_shuffle(Deck *deck, int loops); 


Deck_node *deck_pop_head_node(Deck *deck);

void deck_populate(Deck *deck);

Deck *deck_draw_cards(Deck *deck, int num_cards, int remove); 

void printDeck(Deck *deck);

#endif

below is card.c

//card.c (lib)    
#include <stdlib.h>
#include <stdio.h>
#include "card.h"

Card *card_create(int suit, int number, char *symbol, int symbol_length) {
    Card *card = (Card*)malloc(sizeof(Card) + (sizeof(char)*symbol_length));
    if (card) {
        card->suit = suit;
        card->number = number;
        card->symbol = symbol;
        if (number<=10) card->face = 0;
        else card->face = 1;
    }

    return card;
}

Deck_node* deck_node_add_next(Deck_node *deck_node, Deck_node *next) {
    if (deck_node) {
        deck_node->next = next;
        if (next) next->prev = deck_node;
    }
}

Deck_node *deck_node_create(Card *card) {
    Deck_node *deck_node = (Deck_node*) malloc(sizeof(Deck_node));
    if (deck_node) {
        deck_node->card = card;
    }

    return deck_node;

}

void deck_node_destroy(Deck_node *deck_node) {
    if (deck_node->prev) deck_node->prev->next = deck_node->next;
    if (deck_node->next) deck_node->next->prev = deck_node->prev;

    free(deck_node->card);
    free(deck_node);
}

Deck *deck_create() {
    Deck *deck = (Deck*) malloc(sizeof(Deck));
    deck->size = 0;
    return deck;
}

void deck_destroy(Deck *deck) {
    deck_clear(deck);
    free(deck);
}

void deck_node_switch(Deck_node *n1, Deck_node *n2) {
    if (n1 && n2) {
        Card *tmp = n1->card;
        n1->card = n2->card;
        n2->card = tmp;
    }
}

int deck_random_card_index(Deck *deck) {

    return (rand() % deck->size) + 1;
}

void deck_shuffle(Deck *deck, int loops) {
    for (int i=0; i<loops; i++) {

        int rand1 = deck_random_card_index(deck);
        int rand2 = deck_random_card_index(deck);

        Deck_node *n1 = deck->head;
        Deck_node *n2 = deck->head;

        for(int j=0; j<rand1; j++) {
            n1 = n1->next;
        }

        for (int j=0; j<rand2; j++) {
            n2 = n2->next;
        }

        deck_node_switch(n1, n2);
    }
}

void deck_append_node(Deck *deck, Deck_node *deck_node) {


    if (deck->tail){
        deck_node->prev = deck->tail;
    }
    if (deck->head == NULL) deck->head = deck_node;
    deck_node_add_next(deck_node->prev, deck_node);
    deck->tail = deck_node;
    deck->size+=1;
}

Deck_node *deck_pop_head_node(Deck *deck) { //Emulates taking the first card off the top of deck
    Deck_node *deck_node = (Deck_node*) malloc(sizeof(Deck_node)); 
    Deck_node *next_node = deck->head->next;

    *deck_node = *deck->head;
    deck_node->prev = NULL;
    deck_node->next = NULL;

    free(deck->head);

    deck->head = next_node;
    deck->head->prev = NULL;
    deck->size-=1;
    return deck_node;
}

void deck_populate(Deck *deck) { //Fills deck with cards, assumes deck is empty
    char *symbol_map[] = {"Ace", "2", "3", "4", "5", "6", "7", "8", "9", "10", "Jack", "Queen", "King"};
    for (int i=0; i<52; i+=4) {

        for (int j=0; j<4; j++) {
            Card *card = card_create(j, i/4, symbol_map[i/4], sizeof(symbol_map[i/4])/sizeof(char));

            deck_append_node(deck, deck_node_create(card));
        }
    }
}

Deck *deck_draw_cards(Deck *deck, int num_cards, int remove) {

    Deck *drawn_deck = deck_create();
    int j=0;

    for (int i=0; i<num_cards; i++) {
        deck_append_node(drawn_deck, deck_pop_head_node(deck));
    }

    return drawn_deck;
}

void deck_clear(Deck *deck) { //REMOVES ALL CARDS FROM DECK
    Deck_node *current_node = deck->head->next;
    deck->head = NULL;
    deck->tail = NULL;

    for (int i=0; i<deck->size-1; i++) {
        deck_node_destroy(current_node->prev);
        current_node->prev = NULL;
        current_node = current_node->next;
    }
    deck->size = 0;
}

void printDeck(Deck *deck) {
    //FOR DEBUGGING PURPOSES
    Deck_node *deck_node = deck->head;
    int i=0;
    while (deck_node) {
        printf("Card %d %s\n", i, deck_node->card->symbol);
        i+=1;
        deck_node = deck_node->next;
    }
}

Below is the blackjack demo

//blackjack.c (demo)
#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#include "card.h"


int get_user_input() { //only grabs int from user
    int value = 0;
    char ch;
    while ((ch = getchar()) != '\n' && ch != EOF) {
        if (ch >= '0' && ch<='9'){
            value*=10;
            value+=ch-'0';
        }
        else {
            return value;
        }

    }
    return value;
}

void promptOk(char *msg) {
    printf("%s", msg);
    printf("\n...");
    getchar();
}


int play_round(Deck *deck, Deck *player_cards) {
    system("clear");
    int drawing=1;
    int drawed=0;
    int score=0;
    while(drawing) {
        int sum=0;
        deck_append_node(player_cards, deck_pop_head_node(deck)); 

        Deck_node *current_node = player_cards->head;
        while(current_node) {
            sum+=current_node->card->number+1;
            current_node = current_node->next;
        }
        printf("You drew a %s\n", player_cards->tail->card->symbol);
        printf("Total: %d\n", sum);

        if (sum<21) {
            printf("Draw again? ");

            drawing = get_user_input();
            system("clear");
        }
        else if(sum==21) {
            printf("PERFECT!\n");
            drawing = 0;
        }

        else {
            printf("BUST!\n");
            drawing = 0;
        }
        drawed+=1;
        score = sum;
    }
    return score;
}

void game(Deck *deck, int num_players) {
    Deck **players_cards = (Deck**) malloc(sizeof(Deck*)*num_players);
    while (1) { 
        int player_winning = -1;
        int score_winning = 0;

        for(int player=0; player<num_players; player++) {
            printf("Player %d's turn\n", player+1);
            promptOk("");
            if (!players_cards[player]) players_cards[player] = deck_create(); //if null creates deck
            else deck_clear(players_cards[player]); //otherwises clears all their cards
            int score = play_round(deck, players_cards[player]);
            if (score > score_winning && score <= 21) {
                score_winning = score;
                player_winning = player;
            }
        }

        if (player_winning != -1) printf("Player %d won with a score of %d\n", player_winning, score_winning);
        else printf("Everyone bust this round!");

        printf("Play another round? ");
        if (!get_user_input()) break;
    }

}

int main() {
    srand(time(NULL)); 
    int num_players = 2;
    Deck *deck = deck_create();
    deck_populate(deck);
    deck_shuffle(deck, 4000);

    game(deck, num_players);

    return 0;
}
\$\endgroup\$
5
\$\begingroup\$

Here are some things that may help you improve your program.

Eliminate unused variables

This code declares a variable j in deck_draw_cards and has a parameter remove but then does nothing with either. Your compiler is smart enough to help you find this kind of problem if you know how to ask it to do so.

Ensure every control path returns a proper value

The deck_node_add_next function claims to return a Deck_node* but doesn't. That's an error that should be fixed.

Check the return value of malloc

If the program runs out of memory, a call to malloc can fail. The indication for this is that the call will return a NULL pointer. You should check for this and avoid dereferencing a NULL pointer (which typically causes a program crash). In some places the code properly checks, but in other places it does not.

Don't use system("clear")

There are two reasons not to use system("clear"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named clear, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate this into a seperate function clearscreen() and then modify your code to call those functions instead of system. Then rewrite the contents of that functions to do what you want using C. For example, if your terminal supports ANSI Escape sequences, you could use this:

void clearscreen() {
    printf("\x1b[2J");
}

Initialize structures completely

There is a subtle bug in the program. The main routine calls deck_create() and then deck_populate() which looks normal. Then deck_populate() calls deck_append_node() which checks deck->tail and deck->head before using them. This also looks normal, but the problem is that neither head nor tail was actually initialized to any particular value. I'd recommend adding this line to deck_create:

deck->head = deck->tail = NULL;

There is also an identical issue with deck_node_create. A related but not quite identical issue exists within game(). The code mallocs the players_cards and then checks to see if they're NULL, but they haven't necessarily been initialized yet. I'd recommend simply creating all player hands immediately and then skipping the check within the loop.

Think of the user

The players in this game are referred to as "Player 1" and "Player 2" but the winner is declared to be either "Player 0" or "Player 1" which is not really right. Also, an Ace in blackjack can count as either 1 or 11, which the game currently doesn't understand, so if a player actually draws a blackjack, the program doesn't recognize it as a winning hand! (A Queen+Ace is actually tallied incorrectly as 13!) No blackjack game ever starts with only a single card dealt. Each player starts with two cards, so that's another aspect of the game that could be improved. It's also not at all obvious that when the game asks the user "Draw again?" that what it apparently expects is a number.

Don't leak memory

As currently written, the program is guaranteed to leak memory because both the player's hands and the main deck are never freed. Code to do that should be added, and deck_destroy() should be added to card.h.

Understand sizeof

Right now the code includes this line:

Card *card = (Card*)malloc(sizeof(Card) + (sizeof(char)*symbol_length));

First, sizeof(char) is defined to always be 1, so that's not necessary or useful. Instead, change that to

Card *card = malloc(sizeof(Card) + symbol_length);

Note that no cast is needed here. Another line is this one:

Card *card = card_create(j, i/4, symbol_map[i/4], sizeof(symbol_map[i/4])/sizeof(char));

First, again, sizeof(char) is defined to be one, so it should be omitted. More importantly, the sizeof(symbol_map[i/4]) is returning the size of the pointer and not the size of the string. The code currently only copies only the pointer, so, for example, every Ace in the deck points to the single string.

That's not a bad way to do it, but it means that the length parameter is completely useless. So I'd make the card names const and rewrite deck_populate and card_create, omitting the length parameter and understanding that all that is being stored in a Card is a pointer to an existing name. The revised card_create would look like this:

Card *card_create(int suit, int number, const char *symbol) {
    Card *card = malloc(sizeof(Card));
    if (card) {
        card->suit = suit;
        card->number = number;
        card->symbol = symbol;
        card->face = number>9;
    }
    return card;
}

And the revised deck_populate like this:

void deck_populate(Deck *deck) { 
    static const char *symbol_map[] = {"Ace", "2", "3", "4", "5", "6", "7", "8", "9", "10", "Jack", "Queen", "King"};
    if (deck == NULL) {
        return;
    }
    for (int rank=0; rank < 13; ++rank) {
        for (int suit = 0; suit < 4; ++suit) {
            Card *card = card_create(suit, rank, symbol_map[rank]);
            deck_append_node(deck, deck_node_create(card));
        }
    }
}

Simplify your code

The deck_pop_head_node code currently looks like this:

Deck_node *deck_pop_head_node(Deck *deck) { //Emulates taking the first card off the top of deck
    Deck_node *deck_node = (Deck_node*) malloc(sizeof(Deck_node)); 
    Deck_node *next_node = deck->head->next;

    *deck_node = *deck->head;
    deck_node->prev = NULL;
    deck_node->next = NULL;

    free(deck->head);

    deck->head = next_node;
    deck->head->prev = NULL;
    deck->size-=1;
    return deck_node;
}

That's a lot of code, but it's not quite right (if the last card is given away, deck->tail is not adjusted) and it allocates and frees which is not at all needed. Here's an alternative:

Deck_node *deck_pop_head_node(Deck *deck) { 
    if (deck == NULL || deck->head == NULL) {
        return NULL;
    }
    Deck_node *node = deck->head;
    deck->head = deck->head->next;
    if (deck->head) {
        deck->head->prev = NULL;
    } else {   // just gave away the last card
        deck->tail = NULL;
    }
    node->prev = NULL;
    node->next = NULL;
    --deck->size;
    return node;
}

In a similar vein, the deck_clear code could be shorter and less error prone:

void deck_clear(Deck *deck) { //REMOVES ALL CARDS FROM DECK
    if (deck == NULL) {
        return;
    }
    for (Deck_node *node = deck->head; node; node = deck->head) {
        deck->head = node->next;
        deck_node_destroy(node);
    }
    deck->tail = NULL;
    deck->size = 0;
}

With the existing code, a deck_create() followed immediately by a deck_destroy() causes a segfault and crash on my machine.

Use a better random number generator

You are currently using

return (rand() % deck->size) + 1;

There are a number of problems with this approach. The most significant problem is that the first card (index 0) can never be selected with this code, so the first player always gets dealt an Ace as the first card. (I want to play in your casino!) Another problem is that the low order bits of the random number generator are not particularly random, so neither is the result. On my machine, there's a slight but measurable bias toward 0 with that. See this answer for details, but I'd recommend changing that to

return rand() / (RAND_MAX / (deck->size + 1));

Reconsider your data structure

I don't see much advantage to having a doubly-linked list as the underlying data structure. It seems to me that a plain array would be much easier and faster to manipulate, and make some operations, such as shuffling the deck, very simple.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments: "I didn't know that." or "That's bad advice!" My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard. For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

[...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!) In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for the feedback. I have a couple of questions. I assumed that if a pointer is not set within the structure Init, it is automatically pointed to NULL. I also choose choose to use a double linked list because because when the deck is shuffled, the Deck_nodes just switch card pointers. I assumed that was more efficient. Last but not least, can you give me any insight on the coding format itself? Variable, function, and structure names. Thanks again! \$\endgroup\$ – h3y4w Feb 23 '17 at 15:05
  • \$\begingroup\$ Another answer said there may be something wrong with how I used Malloc in card_create(). Is this true? \$\endgroup\$ – h3y4w Feb 23 '17 at 15:15
  • \$\begingroup\$ In terms of the data structure, something which I considered suggesting but which I forgot to include was to use a circular buffer. There are many games which want access to both ends of the deck and to shuffle but don't otherwise require random access, so it gives a good intermediate position between an array and a linked list. \$\endgroup\$ – Peter Taylor Feb 23 '17 at 15:51
  • \$\begingroup\$ Yes, the malloc in card_create is indeed flawed. I've added a section titled "understand sizeof" in my answer. \$\endgroup\$ – Edward Feb 23 '17 at 16:09
  • \$\begingroup\$ Also, pointers are not automatically equated to NULL or any particular value. Generally speaking, unless you, the programmer. have initialized a variable, it's not initialized to any particular value. There are a few exceptions, but that's a good general guideline. \$\endgroup\$ – Edward Feb 23 '17 at 16:13
3
\$\begingroup\$
typedef struct Card Card;

struct Card{
    int suit;
    int number;
    int face;
    char* symbol;
};

Is there any reason for not inlining this as

typedef struct Card{
    int suit;
    int number;
    int face;
    char* symbol;
} Card;

and similarly for the other structs?


struct Deck_node{
    Deck_node* prev;
    Card* card;
    Deck_node* next;
};

Why Card* and not Card?


struct Deck{
    Deck_node* head;
    Deck_node* tail;
    int size;
};

Are you sure you want to keep size? It allows you to optimise some uses of the deck, but means that you have to ensure that it's kept up to date. At present the API doesn't do that. In particular, by exposing

Deck_node* deck_node_add_next(Deck_node *deck_node, Deck_node *next);

in the header you allow people to break your internal state.


Card *card_create(int suit, int number, char *symbol, int symbol_length) {
    Card *card = (Card*)malloc(sizeof(Card) + (sizeof(char)*symbol_length));

There's definitely a misunderstanding here, although I'm not sure quite what it is. Only the first sizeof(Card) bytes of the allocated memory are actually used. If the intention was for Card to be variable size then there's probably some way of doing it with char[], but I'm almost as out of my depth here as you.


Deck_node* deck_node_add_next(Deck_node *deck_node, Deck_node *next) {
    if (deck_node) {
        deck_node->next = next;
        if (next) next->prev = deck_node;
    }
}

This is buggy. It would work for a singly linked list, but Deck_node is a doubly-linked list.


void deck_shuffle(Deck *deck, int loops) {
    for (int i=0; i<loops; i++) {

        int rand1 = deck_random_card_index(deck);
        int rand2 = deck_random_card_index(deck);

        Deck_node *n1 = deck->head;
        Deck_node *n2 = deck->head;

        for(int j=0; j<rand1; j++) {
            n1 = n1->next;
        }

        for (int j=0; j<rand2; j++) {
            n2 = n2->next;
        }

        deck_node_switch(n1, n2);
    }
}

I see two big problems with this implementation. Firstly, it's not uniform, which is something that I would expect users of the library to insist on. Secondly, it's not efficient. In the worst case it walks the entire length of the deck every time.

I would suggest removing the loops parameter, and using the following approach:

  1. Allocate some temporary memory to hold a Deck_node* for each card in the deck.
  2. Walk the list once to fill the temporary space with a pointer to each Deck_node.
  3. Fisher-Yates shuffle the temporary space.
  4. Rewrite all of the next and prev pointers, as well as deck->head and deck->tail, to reproduce the shuffled order.
  5. Free the temporary memory.

void deck_populate(Deck *deck) { //Fills deck with cards, assumes deck is empty
    ...
    for (int i=0; i<52; i+=4) {
        ...
            Card *card = card_create(j, i/4, symbol_map[i/4], sizeof(symbol_map[i/4])/sizeof(char));
        ...
    }
}

Why not just for (int i=0; i<13; i++)?


Deck *deck_draw_cards(Deck *deck, int num_cards, int remove) {

remove isn't used in the function body. Should it be used? If not, remove it from the parameters.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your feedback. Concerning why I used typedef as a separate statement, it was giving me compiling errors saying it was not defined in the Deck_node and Deck structs. Any insight on variable and function names as well as overall code format? Thanks in advance. \$\endgroup\$ – h3y4w Feb 23 '17 at 15:13
  • \$\begingroup\$ @h3y4w, I'm not really a C programmer and I don't know what is considered good style in those areas, or to what extent it's a free-for-all and you should just be consistent within your project. \$\endgroup\$ – Peter Taylor Feb 23 '17 at 15:52
  • \$\begingroup\$ "Is there any reason for not inlining this as", coding style. \$\endgroup\$ – Stargateur Feb 23 '17 at 22:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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