18
\$\begingroup\$

I am (trying) to learn the C programming language for fun with my father's book "The C Programming Language". I tried writing a tic-tac-toe game in C. I have no professional experience, so I do not know whether the following is how real code is that real programmers write. I ask for your judgement.

#define BOARDSZ         9
#define BOARDLEN        3

#define PLYR_NO         0
#define PLYR_X          1
#define PLYR_O          2
#define PLYR_1ST        1

#define X               0
#define O               1
#define DRAW            2
#define X_WON           3
#define O_WON           4

int g_board[9];
int g_status;

check()
{
        register i, off;
        auto p;
        /* Draw */
        for (i = 0; i < BOARDSZ; i++) {
                if (g_board[i] == PLYR_NO)
                        goto check_win;
        }
        g_status = DRAW;
        return;

check_win:
        /* Horizontal */
        for (i = 0; i < BOARDLEN; i++) {
                off = BOARDLEN * i;
                if (g_board[off + 0] == PLYR_NO)
                        continue;
                if (g_board[off + 0] != g_board[off + 1])
                        continue;
                if (g_board[off + 0] != g_board[off + 2])
                        continue;
                p = g_board[off + 0];
                goto set_win;
        }
        /* Vertical */
        for (i = 0; i < BOARDLEN; i++) {
                if (g_board[i + 0] == PLYR_NO)
                        continue;
                if (g_board[i + 0] != g_board[i + BOARDLEN])
                        continue;
                if (g_board[i + 0] != g_board[i + BOARDLEN * 2])
                        continue;
                p = g_board[i + 0];
                goto set_win;
        }
        /* Diagonal */
        /* \ */
        if (g_board[0] == PLYR_NO)
                goto nextdiag;
        if (g_board[0] != g_board[4])
                goto nextdiag;
        if (g_board[0] != g_board[8])
                goto nextdiag;
        p = g_board[0];
        goto set_win;
nextdiag:
        /* / */
        if (g_board[2] == PLYR_NO)
                goto end;
        if (g_board[2] != g_board[4])
                goto end;
        if (g_board[2] != g_board[6])
                goto end;
        p = g_board[2];
        goto set_win;
set_win:
        if (p == PLYR_X)
                g_status = X_WON;
        else
                g_status = O_WON;
end:
        return;
}

init()
{
        register i;
        for (i = 0; i < BOARDSZ; i++)
                g_board[i] = PLYR_NO;
        g_status = PLYR_1ST;
}

/* Return
 *      0 if success
 *      1 if position is already taken
 *      2 if game is finished
 */
play(pos)
{
        if (g_status == X_WON || g_status == O_WON || g_status == DRAW)
                return 2;

        if (g_board[pos] != PLYR_NO)
                return 1;
        if (g_status == X) {
                g_board[pos] = PLYR_X;
                g_status = O;
        }
        else {
                g_board[pos] = PLYR_O;
                g_status = X;
        }

        check();
        if (g_status == X_WON || g_status == O_WON || g_status == DRAW)
                return 2;
        return 0;
}

printbrd()
{
        register i, j;
        auto p, pos;
        char c, chX, chO;
        chX = 'X';
        chO = 'O';
        for (i = 0; i < BOARDLEN; i++) {
                for (j = 0; j < BOARDLEN; j++) {
                        pos = i * BOARDLEN + j;
                        p = g_board[pos];
                        if (p == PLYR_NO)
                                c = pos + '1';
                        else if (p == PLYR_X)
                                c = chX;
                        else
                                c = chO;
                        printf("%c", c);
                }
                printf("\n");
        }
}

/* if a read error, occurs, exits 1. */
char xgetchar()
{
        auto c;
        c = getchar();
        if (c == -1 || c == 0) {
                printf("Fatal: Could not read input\n");
                exit(1);
        }
        /* Empty the buffer, to only consider the first char, not enter. */
        if (c != '\n')
                while (getchar() != '\n');
        return c;
}

/* Return if the user confirmed or refused. */
getyn(msg)
        char *msg;
{
        char c;
        while (1) {
                printf("%s [Y/N]: ", msg);
                c = xgetchar();
                if (c == 'y' || c == 'Y')
                        return 1;
                if (c == 'n' || c == 'N')
                        return 0;
        }
}

game()
{
        char c;
        int pos, r;
        init();
        printbrd();
        while (1) {
                c = xgetchar();
                if (c == 'q' || c == 'Q') {
                        printf("Byte\n");
                        return;
                }
                if (c == '\n') {
                        printbrd();
                        continue;
                }
                if (c < '1' && c > '9') {
                        printf("Bad input\n");
                        continue;
                }
                pos = c - '1';
                r = play(pos);
                if (r == 0) {
                        printbrd();
                        continue;
                }
                if (r == 1) {
                        printf("Position already taken\n");
                        printbrd();
                        continue;
                }
                if (r == 2) {
                        break;
                }
        }

        printbrd();
        if (g_status == X_WON)
                printf("X Won!\n");
        else if (g_status == O_WON)
                printf("O Won!\n");
        else
                printf("It is a draw...\n");
}

main()
{
        while (1) {
                game();
                if (!getyn("New game?"))
                        break;
                printf("\n");
        }
        return 0;
}
\$\endgroup\$
13
  • 1
    \$\begingroup\$ What does BOARDSZ mean? Try to name your variables in a way such that it will make others understand its intended meaning. This will make your code easier to understand and work with. \$\endgroup\$ Apr 15 at 0:41
  • 1
    \$\begingroup\$ @NapoleonBonaparte, thank you for reading my question. It means "board size". It is because only the first 8 characters are used. The rest is ignored, otherwise I'd used "BOARDSIZE". \$\endgroup\$
    – Uriel
    Apr 15 at 0:44
  • 1
    \$\begingroup\$ Is there a reason why you can't name your variables (in this case, constants) with a more meaningful and descriptive names? I use to do the same with my code; I soon found out that such practice will make debugging a lot harder. \$\endgroup\$ Apr 15 at 0:49
  • 7
    \$\begingroup\$ That book is a really important historical document, but it is NOT useful to learn modern C in 2024. Even C has changed. Please. Please! find a newer source than something that was last updated in 1988. \$\endgroup\$
    – bitmask
    Apr 15 at 10:37
  • 4
    \$\begingroup\$ I believe the book OP is using is not even the one describing C89, but the K&R C version that preceded ANSI C. \$\endgroup\$
    – Harith
    Apr 15 at 13:39

3 Answers 3

19
\$\begingroup\$

Unfortunately, it seems like you have stumbled across an older version of The C Programming Language which described a dialect of C now dubbed K&R C. K&R C was superseded by ANSI C (C89), and has removed much of cruft prevalent in K&R C. As such, your code will fail to compile on any standard conforming compiler (at the very least, you will have to disable a bunch of standard features) and your implementation would generally be considered poor 'modern C' .


No 8 character limit for identifiers

ANSI C does not have an 8 character limit for identifiers. So you don't need to constrain your identifiers to names such as BOARDSZ, printbrd, getyn, et cetera. In fact, a much appreciated convention in programming to have descriptive names for identifiers to help other programmers' readability.


register and auto

The register keyword, while still valid, is pretty much useless. It was originally designed to aid compilers as a hint that a particular variable should be stored in a CPU register. Modern compilers are very smart and automatically determine that; in fact, most (if not all) compilers will ignore the register keyword. The alternative is to declare the variable with its type: int i, off.

Similarly, the auto keyword was used by programmers to specify that a particular variable is local to the scope. It is a holdover from B, the predecessor of C, and (again) is pretty much useless since function local variables, by default, have automatic storage. Again, the alternative is to declare it using its type: int p


Specifying the return type

C99 and later do not allow implicit return types (i.e., functions without a return type) and compilers will throw a big fat warning if you do so. Make sure you specify the return type when declaring a function.

int check() {
  ...
} 

int init() {
  ...
}

Old style parameter declarations

Old style parameter declarations are generally considered bad form.

getyn(msg)
        char *msg; // Not good!

You will still see old C code using this style, but this has been superseded by declaring the arguments inside the function prototype.

int getyn(char* msg) // Good!

Now, coming to your implementation:

Checking for the win

At each turn, you are checking the entire board for a winner. You can optimize this out by keeping track of the current player ('X' or 'O') and only checking the row and column the move was made in, as well as the diagonals, since the rest of the board is unchanged and we already know that there was no winner before the current turn.


Other comments

  • Include stdio.h for printf and getchar. ANSI C does not allow implicit function calls.

  • Use braces for single statement conditional and/or loop blocks. Without braces, they can become error prone and harder to read.

    if (c != '\n')
                 while (getchar() != '\n');
    

    can be replaced with:

    if (c != '\n') {
        while (getchar() != '\n') {}
    }
    
\$\endgroup\$
3
  • 2
    \$\begingroup\$ You can optimize this out by keeping track of the current player ('X' or 'O') and only checking the row and column You also need to check the diagonal right? \$\endgroup\$
    – Exprove
    Apr 15 at 15:38
  • 2
    \$\begingroup\$ Yes, I missed that. Updated. \$\endgroup\$
    – Rish
    Apr 15 at 16:05
  • \$\begingroup\$ Hello, Rish. I do not know this question would get so much attention! Thank you a lot for this response. I imagined it was old and outdated, because other code in the internet was quite different, but now I know. The difference seems to be small, so it will be fine to learn. Thank you, again! \$\endgroup\$
    – Uriel
    Apr 19 at 20:02
6
\$\begingroup\$

Advice I - stdio.h:

I was able to compile your code in Visual Studio only via adding #include <stdio.h>. Please, add it to your main.c.

Advice II You have:

int g_board[9];

Why not:

int g_board[BOARDSZ];

Advice III - Get rid of labels

Labels - such as check_win - make the code harder to reason about. I suggest you fix your logic such that you don't need them.

Advice IV - Make a board a data type I had something like this in mind:

typedef struct {
    char data[BOARDSZ];
} board_t;

board_t* board_t_alloc() {
    board_t* b = malloc(sizeof(board_t));
    memset(b->data, 0, BOARDSZ);
    return b;
}

int board_t_is_marked(board_t* b, size_t x, size_t y) {
    return b->data[3 * y + x];
}

void board_t_mark(board_t* b, size_t x, size_t y, int player) {
    b->data[3 * y + x] = player;
}
.
.
.
void board_t_print(board_t* b) {
    // Your rendering logic.
}
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Suggestion: board_t* b = malloc(sizeof(board_t)); memset(b->data, 0, BOARDSZ); replaceable with b = calloc(1, sizeof *b);. \$\endgroup\$ Apr 15 at 23:42
  • \$\begingroup\$ That is very interesting, but since I have just one game, using global variables seem better. Still, I will try to code it with your method. Thanks! \$\endgroup\$
    – Uriel
    Apr 19 at 20:03
3
\$\begingroup\$

Goto Considered Harmful

With goto, you have stumbled into one of programming's oldest debates (and potentially one of its oldest memes!): if and when goto is an acceptable solution to a problem.

In C, I believe goto has a place in specific cases, especially for error checking and cleanup. In many higher level languages, goto does not exist or is highly discouraged, and you're expected to implement that logic using if-else/for/while or nested functions. Learn to avoid it when you can.

Take advantage of short-circuiting!

The C compiler (or at least, every modern compiler) will use short-circuit evaluation of || and &&. That means that if you do a && b, if a is false, then b will skip evaluating, because a && b can never be true. You can use this to avoid some of the continue logic, while getting the same effect of not checking later squares if you know you won't get a win.


Let's look at how you could restructure part of your code:

/* Horizontal */
for (i = 0; i < BOARDLEN; i++) {
        off = BOARDLEN * i;
        if (g_board[off + 0] == PLYR_NO)
                continue;
        if (g_board[off + 0] != g_board[off + 1])
                continue;
        if (g_board[off + 0] != g_board[off + 2])
                continue;
        p = g_board[off + 0];
        goto set_win;
}
int check_horizontal()  // Returns a player if they have won horizontally, otherwise NO_RESULT.
{
    for (int i = 0; i < BOARDLEN; i++) {
        int off = BOARDLEN * i;
        if (
            g_board[off + 0] != PLYR_NO 
            && g_board[off + 0] == g_board[off + 1]
            && g_board[off + 0] == g_board[off + 2]
        ) {
            return g_board[off + 0];
        }
    }
    return NO_RESULT;
}

When this evaluates, first the machine will check g_board[off + 0] != PLYR_NO. If this is false (i.e. the first square IS PLYR_NO), we know the entire && can never be true, so we can skip it. The evaluation of the if statement will immediately stop, and execution will reach the end of the for-loop and continue on the next iteration. (So this is exactly the same as the continue;)

Then, the return takes the place of our goto and setting p: if we find a win, the loop immediately stops, and we return the winner.

Since we've removed the goto that explicitly tells execution where to go next, we could reuse check_horizontal() in other contexts across other functions. We have to change the rest of our check() function to use this function, of course, but the end result generally looks much cleaner:

int check()
{
    /* Horizontal */
    int horizontal_result = check_horizontal();
    if (horizontal_result != NO_RESULT) {
        return horizontal_result;
    }
    /* Vertical */
    int vertical_result = check_vertical();
    if (vertical_result != NO_RESULT) {
        return vertical_result;
    }
    /* Diagonal */
    int diagonal_result = check_diagonal();
    if (diagonal_result != NO_RESULT) {
        return diagonal_result;
    }
    if (check_draw()) {
        return DRAW;
    }
    return NO_RESULT;
}
\$\endgroup\$
1
  • \$\begingroup\$ I agree with the other answers, and there's plenty of other nitpicks here and there. (if you decide to make a second version I'm sure people will help with more of those nitpicks.) \$\endgroup\$
    – Kaia
    Apr 16 at 2:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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