3
\$\begingroup\$

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);
\$\endgroup\$
3
\$\begingroup\$

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>
\$\endgroup\$
  • \$\begingroup\$ Hi @chux, I have definitely learned a lot from this answer. One question I had - what does "include twice detection" mean? Why would you suggest I include "rng.h" twice, and which file would I include it twice in? I have a "#pragma once" guard in place to protect against double inclusion. Does this notation help make something clear? \$\endgroup\$ – Faraz Jan 17 at 19:11
  • \$\begingroup\$ @Faraz An include file should not need to be included twice, yet include files should tolerate being included twice, hence code guards. So if you inadvertently made an include file that was not tolerant of being included twice, how would that error be detected? The error might exist a long time before some code did so - a latent bug ( I despise those sleeping bugs.). By having xxx.c purposely include xxx.h twice, that tolerance is tested when the coder is messing with xxx.c/xxx.h. Of course other .c files need not purposely include xxx.h twice. \$\endgroup\$ – chux - Reinstate Monica Jan 17 at 19:20
  • \$\begingroup\$ @Faraz Note again #pragma once is not standard, even though fairly common. Also if your test code did not have a #pragma once or code guard, how would this compilation fail? Perhaps not at all. If a #pragma once or code guard is important, it is useful for the compilation of xxx.c (or test code for xxx.c,xxx.h) to detect it. \$\endgroup\$ – chux - Reinstate Monica Jan 17 at 19:26
  • \$\begingroup\$ Ok, and one other question - if I change sizeof(words)/sizeof(words[0]) into a function-like macro such as #define len(x) (((sizeof(x)))/(sizeof(x[0]))), would you suggest placing it in the utils.h file or main.c from where it is being called? \$\endgroup\$ – Faraz Jan 17 at 19:42
  • \$\begingroup\$ @Faraz len is too generic a name to be in any .h file. #define should () the x in both places: perhaps as .(sizeof(x)/sizeof((x)[0])) or (sizeof(x)/sizeof(*(x))). \$\endgroup\$ – chux - Reinstate Monica Jan 17 at 20:44

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.