Hangman game in C

Question

I am a beginner who wrote this simple Hangman game in C for fun and to practice programming. I am looking for advice on optimizing this code and making it adhere to best practices. Are there too many variables, conditionals, and loops?

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <stdbool.h>

int main(void)
{
    //get a word
    char words[12][10] = {"depend", "rich", "twig", "godly", "fang", "increase", "breakable", "stitch", "pumped", "pine", "shrill", "cable"};
    srand(time(NULL));
    int r = rand() % 12;
    char* word = words[r];

    int length = strlen(word);
    int maxIncorrect = 5;
    char correct[26] = {'\0'};
    char incorrect[26] = {'\0'};
    int amountCorrect = 0;
    int amountIncorrect = 0;

    //repeat until maxIncorrect wrong guesses
    while (amountIncorrect < maxIncorrect)
    {
        char guess = '\0';
        bool inWord = false;
        bool allCorrect = true;
        printf("\n________________________________________\n\n");

        //print blanks and correcly guessed letters
        for (int i = 0; i < length; i++)
        {
            inWord = false;
            for (int j = 0; j < 26; j++)
            {
                if (word[i] == correct[j])
                {
                    inWord = true;
                }
            }
            if (inWord)
                {
                    printf("%c ", word[i]);
                }
            else
            {
                printf("_ ");
                allCorrect = false;
            }
        }

        //stop if all letters have been correctly guessed
        if (allCorrect)
        {
            printf("\n\nCorrect!\n");
            return 0;
        }

        //print incorrect guesses
        printf("\n\nIncorrect guesses: ");
        for (int i = 0; i < amountIncorrect; i++)
            {
                printf("%c", incorrect[i]);
            }

        printf("\n");

        inWord = false;
        bool valid = true;
        printf("\n%i incorrect guesses remaining\n", maxIncorrect - amountIncorrect);
        printf("Enter a single lowercase letter: ");

        //get user's guess and check if it is valid (a lowercase letter that has not been guessed before)
        scanf("%c%*c", &guess);
        for (int i = 0; i < amountCorrect; i++)
        {
            if (guess == correct[i])
            {
                valid = false;
            }
        }
        for (int i = 0; i < amountIncorrect; i++)
        {
            if (guess == incorrect[i])
            {
                valid = false;
            }
        }
        if (guess < 97 || guess > 122)
        {
            valid = false;
        }

        //go back to top of loop guess is invalid
        if (!valid)
        {
            printf("\nInvalid\n");
        }

        //check if guess is part of the word or not
        else
        {
            for (int i = 0; i < length; i++)
            {
                if (guess ==  word[i])
                {
                    inWord = true;
                }
            }
            if (inWord == true)
            {
                correct[amountCorrect] = guess;
                amountCorrect++;
            }
            else
            {
                incorrect[amountIncorrect] = guess;
                amountIncorrect++;
            }
        }
    }

    printf("\nThe word was %s\n", word);
    return 0;
}

Answer 1

General Overview

This is pretty good for a beginner.

I see some best practices are already being followed such as placing single statements in an if statement within { and } as well as putting each variable on a new line and initializing it. The variable names are quite understandable.

The code could be more flexible and more modular.

When developing code it is a good idea to compile with switches that report possible errors such as the GCC -pedantic flag. When I compile this code I get the following warning messages:

• main.c(11,11): warning: 'function': conversion from 'time_t' to 'unsigned int', possible loss of data
• main.c(15,18): warning: 'initializing': conversion from 'size_t' to 'int', possible loss of data

Complexity

A general best practice in almost every programming language is that no function should be larger than a single screen in the editor or IDE. This generally means no function should be larger than 55 to 60 lines of code, in the current implementation the main function is 118 lines of code. The reason for this particular best practice is that anything more than one screen makes it very difficult to follow the logic of the function.

The function main() is too complex (does too much). As programs grow in size the use of main() should be limited to calling functions that parse the command line, calling functions that set up for processing, calling functions that execute the desired function of the program, and calling functions to clean up after the main portion of the program. In the current code main is the

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

An example of code that could be a function:

        //print blanks and correcly guessed letters
        for (int i = 0; i < length; i++)
        {
            inWord = false;
            for (int j = 0; j < 26; j++)
            {
                if (word[i] == correct[j])
                {
                    inWord = true;
                }
            }
            if (inWord)
            {
                printf("%c ", word[i]);
            }
            else
            {
                printf("_ ");
                allCorrect = false;
            }
        }

Magic Numbers

There are Magic Numbers in the code function (12, 10, 5, 26), it might be better to create symbolic constants for them to make the code more readable and easier to maintain. These numbers may be used in many places and being able to change them by editing only one line makes maintenance easier.

Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. There is a discussion of this on stackoverflow.

Flexibility

This code at the very beginning of main does not allow for additional words to be added to the list:

    char words[12][10] = { "depend", "rich", "twig", "godly", "fang", "increase", "breakable", "stitch", "pumped", "pine", "shrill", "cable" };
    srand(time(NULL));
    int r = rand() % 12;
    char* word = words[r];

A flexible alternative would be:

    //get a word
    const char *words[] =
        {
            "depend",
            "rich",
            "twig",
            "godly",
            "fang",
            "increase",
            "breakable",
            "stitch",
            "pumped",
            "pine",
            "shrill",
            "cable" 
        };
    size_t wordCount = sizeof(words) / sizeof(*words);
    size_t r = rand() % wordCount;
    char* word = words[r];

This will allow you to add all the words you want and then calculate the size of the array of words.

*Note: The above code could be a function, but seeding the random number generator should probably be one of the first lines in the main function. *

Answer 2

We don't deal very well with input errors:

    scanf("%c%*c", &guess);

When we ignore the return value from scanf(), we lose the ability to deal with stream problems such as end of input.

There's an inbuilt assumption here that the user will behave well and enter exactly one letter followed by newline. More adversarial testers will enter some blank lines or multiple characters, breaking this assumption and causing the program to get out of sync.

It might be best to read a character at a time, using getc() (remembering to test against EOF before narrowing to char), and silently ignore non-alphabetic input such as newline.

---

This test implies we're targeting ASCII systems:

    if (guess < 97 || guess > 122)

On systems using other encodings (notably EBCDIC), this will reject letters and allow many non-letters. We might want to use isalpha() from <ctype.h> to validate input (don't forget to cast to unsigned char for this function).

It would be helpful to players if we don't penalise for entering an already-guessed letter - consider remembering the user's wrong guesses for them.