Hangman written in C

Question

I would like your feedback on any improvements that can be made to this Hangman game I have written in C. Specifically, improvements in terms of runtime and code organization. This game was a nice way for me to learn more about the features of the C language, specifically pointers, and thus I have heavily commented the code I have written for the purposes of learning.

Below are the source files and the CMakeLists file (which includes many runtime Clang sanitizers enabled). Alternatively, the code can be easily compiled with the following: cc *.c -o hangman && ./hangman

main.c

/**
 *                      * Hangman in C *
 * O(1) lookup using pointers to 26 letters which each have a state
 * A letter is either _ (not picked, default), or the letter itself
 * I was inspired by seeing many other Hangman implementations which
 * relied on a multiple layers of iteration, this aims to be 'simple'
 * and 'idiomatic', by using a different approach.
 *
 * @date 1/15/19
 * @author Faraz Fazli
 */

#include <stdio.h>
#include <string.h>
#include <ctype.h>
#include <stdlib.h>
#include "rng.h"
#include "utils.h"

/**
 * Use enum to replace "magic numbers" instead of #define or const
 * Ref: Practice of Programming, p.21
 */
enum {
    ALPHABET_SIZE = 26,
    TOTAL_TRIES = 10,
};

// Words the program chooses from
static char *words[] = {"racing", "magic", "bow", "racecar"};

int main() {
    char letters[ALPHABET_SIZE];

    // Let's set 'letters' to be composed of just _
    // This will later be changed as the user guesses
    memset(letters, '_', ALPHABET_SIZE);

    init_rng();

    // Total number of elements in our array
    size_t total_elems = sizeof(words)/sizeof(words[0]);

    char *word = words[rand_to(total_elems)];
    size_t word_size = strlen(word) + 1; // includes NUL character

    // Here I used 'malloc' instead of VLA
    char **word_to_guess = malloc(word_size * sizeof(word));
    size_t word_len = strlen(word); // excludes NUL

    // point each element to the appropriate letter in our array
    for (size_t i = 0; i < word_len; i++) {
        word_to_guess[i] = &letters[from_a(word[i])];
    }

    int tries = 0;
    size_t num_previous_underscores = word_len;

    print_count_underscores(word_to_guess);
    fputs("\nPick a letter: ", stdout);

    // Could replace getchar() with fgets and parse each letter
    // which may serve better in the future
    int current_letter;
    while ((current_letter = getchar()) != EOF) {
        if (!isalpha(current_letter)) {
            // Simply continue - printing here causes bugs
            continue;
        }

        // convert to lower case
        current_letter = tolower(current_letter);

        // distance from 'a'
        size_t letter_pos = from_a(current_letter);

        // Letter has already been picked if it is in array
        if (letters[letter_pos] == current_letter) {
            puts("Please pick a different letter.");
            continue;
        } else {
            // Change underscore to the letter
            letters[letter_pos] = (char) current_letter;
        }

        // Finds if word still has underscores, and print word state
        size_t num_underscores = print_count_underscores(word_to_guess);

        // If letter has no correct guesses from this turn, increment tries
        if (num_underscores == num_previous_underscores) {
            tries++;
        }
        num_previous_underscores = num_underscores;

        // Win if no underscores left
        if (num_underscores == 0) {
            puts("-> YOU WIN!");
            break;
        }

        if (tries < TOTAL_TRIES) {
            printf("\nTries Remaining: %d\n", TOTAL_TRIES - tries);
            fputs("Pick a letter: ", stdout);
        } else {
            puts("No tries left! Game Over!");
            break;
        }
    }
    free(word_to_guess);
}

rng.c

#include "rng.h"
#include <time.h>

void init_rng(void) {
    srand((unsigned int) time(NULL));
}

size_t rand_to(size_t max) {
    return (unsigned long) rand() / (RAND_MAX / max + 1);
}

rng.h

#pragma once
#include <stdlib.h>

// Initializes random number generator
void init_rng(void);

/**
 * Helper method for Random Number Generation
 * @param max - max number
 * @return between 0 to max
 */
size_t rand_to(size_t max);

utils.c

#include <stdio.h>
#include "utils.h"

size_t print_count_underscores(const char **word_to_guess) {
    size_t num_underscores = 0;
    while (*word_to_guess) {
        printf("%c ", **word_to_guess);
        if (**word_to_guess++ == '_') {
            num_underscores++;
        }
    }
    return num_underscores;
}

size_t from_a(int letter) {
    return (size_t) abs(letter - 'a');
}

utils.h

#pragma once

#include <stdlib.h>

/**
 * Prints the state of each letter and counts the number of underscores
 * @param word_to_guess - word being guessed (array of pointers)
 * @return underscore count
 */
size_t print_count_underscores(const char **word_to_guess);

/**
 * Returns the distance from 'a'
 * @param letter 'a' to 'z'
 * @return 0 through 25
 */
size_t from_a(int letter);

Answer 1

Overall

A quality effort.

I especially like function documentation in the header files, where others can see it.

---

#pragma once

#pragma once is not standard C. For higher portability, use code guards.

---

Mostly minor stuff follows

Style

char **word_to_guess = malloc(word_size * sizeof(word)); looks wrong.
Suggest char **word_to_guess = malloc(word_size * sizeof *word_to_guess);

Trouble with letters with locales

Only important when outside C locale.

In various locales letters[letter_pos] will become UB as letters[] is only good for the usual 26. Defensive coding would insure a valid index.

In such locales letters[letter_pos] can have a negative value while the "same" current_letter is positive. Then if (letters[letter_pos] == current_letter) { will fail, even with the "same" letter.

Perhaps cast:

// if (letters[letter_pos] == current_letter) 
if (letters[letter_pos] == (char) current_letter) 

len() missing

len(words) needs a definition.

Namespace

rng.h declares init_rng and rand_to. Consider rng_init(), rng_to().

One too many types, maybe

rand_to() involves 3 types: size_t, unsigned long, int. The selection of (unsigned long) seems arbitrary. Recommend to use size_t. Note this code is weak unless RAND_MAX is much larger than max.

size_t rand_to(size_t max) {
  return (unsigned long) rand() / (RAND_MAX / max + 1);
  return (size_t) rand() / (RAND_MAX / max + 1);
}

Deeper: size_t is a tricky type as its rank is not defined in relationship to the other unsigned types. size_t is often unsigned long, yet code should not rely on that. In this case we know [0 <= rand() <= INT_MIN <= UINT_MAX] and [32767 <= RAND_MAX <= INT_MIN <= UINT_MAX] and so could be more careful with the below, which is not to far off your original code. The care comes from (size_t) rand() could, in theory on a unicorn platform, truncate the value. A (unsigned) cast will never lose info here and get us into the nicely behaviored unsigned math. It is not excessive, like unsigned long may be. The division will push the math to wider unsigned types as needed.

  return (unsigned) rand() / ((unsigned) RAND_MAX / max + 1u);

Minor: #include

I'd expect <stdlib.h> in rng.c. IMO, the standard includes found in the matching "rng.h" should not be relied on. As with such style issues: code to you group's coding guidelines.

#include "rng.h"
#include <stdlib.h>  // add
#include <time.h>

include twice detection.

Take this idea or not: It is useful to detect if the matching header file suffers redundant inclusion as below. Within xxx.c, and only here, I find including xxx.h twice nicely exercises this.

#include "rng.h"
#include "rng.h"  // add
#include <stdlib.h>
#include <time.h>