UNO for ncurses

I've been teaching myself C for the last few months. As a learning exercise, I set out to write an ncurses implementation of the classic card game Uno, mostly because it was was one of the simpler card games I could think of.

Since my emphasis in writing this was primarily learning concepts and algorithms, the interface is pretty minimal, and there aren't a lot of bells and whistles beyond what's needed to play the game. Nonetheless, I've tried to implement the rules and gameplay dynamics of the card game as closely as possible, so things like saying "Uno" and challenging Draw Four Wilds are included. For the most part, my goal was to make the game as lightweight, efficient, and airtight as I could.

In the header I define a Card type which contains values representing the number/symbol and color of that card, and then an array of Card constants for every card in the deck. Most of the program, though, works through Handcard structs, which contain a pointer to a Card constant as well as a pointer to another Handcard. This way, each player's hand, as well as discard and draw piles, are singly linked lists, to which memory is dynamically allocated and deallocated every time a card is drawn or played.

For the most part, I'm happy with how the game came out. To my knowledge all the memory leaks have been closed, and most of the bugs have been stamped out too. It's not perfect: everything is written to the standard ncurses window, so updating different parts of the user interface is a little clumsy. Also, I couldn't find an ncurses equivalent for fgets, so when entering players' names and backspacing the cursor behaves a little funny (though it still deletes characters properly.)

My main questions basically revolve around whether my implementation follows best practices, and whether I could have implemented certain things better, in particular the data structures for organizing cards, and the layered gameplay loops. I've also not gotten around to writi

-
Your shuffling-algorithms is broken: See The Danger of Naïveté for the explanation, and Fisher-Yates shuffle on wikipedia for a working replacement.

Aside from that, you really should only initialize the random-generator once, at the start of the program.

(For debuggability, you might want to add an option to specify the seed on the command-line.)

-
For no obvious reason, you have separated definition and initialization of multdraw, noskip and winscore in main...

-
You could use getopt for parsing the command-line. As-is, it looks complicated enough to extract into its own (local) function.

-
Actually, you should really break it up in more functions. If they are only used in the same TU, mark them static. And if you define them before you use them, you don't even need any forward-declaration.

-
Your handling of cards is far too complicated.

A deck consists of:

-
Wild card, and Wild card draw four, each four times.

The wild-cards are special in that they don't actually have a color. Whoever plays one decides which color follows. Best to model that by letting them assume that color.

• 0 once per color.

• skip, reverse, draw two, 1 to 9, each twice per color.

All cards of same color and symbol are indistinguishable, their identities are immaterial.

So, let's encode them:

• We have four colors, put that in the low bits.

• And we have 15 symbols, declaring symbol 0 invalid makes 16.

6 bits comfortably fit into a char...

And now we can allocate a single buffer for each hand/deck/pile at the start, and use the first byte as a count. Much easier and faster.

-
You really should test whether scanw and other input-functions actually succeed in getting the asked number of inputs, not only whether those inputs are in the range you accept...

-
sizeof(char) is defined as 1, also Don't cast the result of malloc/calloc, and avoid useless zeroing of buffers.

-
return 0; is implicit in main since C99.

-
Your handling of single-linked-lists can be improved. Not that you should use any single-linked-list in that program, a counted or 0-terminated string is far superior...

void clean(struct Handcard* head) { // Was quadratic. Now optimal
    for(struct Handcard* next; head; head = next) {
        next = head->link;
        free(head);
    }
}

-
I wonder why you added the superfluous null-check in length...

-
Regarding struct Cardname, I earnestly suggest you not use a union but always two strings.

StackExchange Code Review Q#109557, answer score: 5