2
\$\begingroup\$

This was the first game I made not too long ago (2018), and while I'm working on another, more complex version, I want to know how good or bad I did this one and what can be improved.

Note: the change of players and detection of free spaces are the things I did with help of a friend, everything else I did it myself.

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

int printBoard();
int clear(int h, int w);
int freeSpace();
int player();
int checkLine(int x, int y, int z);
int winner();

int player_input;
char board[9] = {' ', ' ', ' ',
                 ' ', ' ', ' ',
                 ' ', ' ', ' '};

int player_num = 1;
char symbol = 'X';

int main(void)
{
    char start;

    clear(24, 80);

    printf("\nTotito game on a 3x3 board:\n");
    printf("Enter number from 1 to 9 to mark the respective spaces in the board.\n");
    printf("Press a number to start the game: ");
    scanf("%c", &start);

    clear(24, 80);
    printBoard();

    while (true)
    {
        player();
        clear(24, 80);
        printBoard();
        winner();

        player_num = (player_num == 1)? 2: 1;
        symbol = (player_num == 1)? 'X': 'O';
    }
}

int printBoard() // 3x3 board
{
    int lines = 7;
    int columns = 7;
    int n = 0;

    for (int i = 1; i <= lines; i++)
    {
        printf("    ");

        for (int j = 0; j < columns; j++)
        {
            if (i % 2 != 0)
            {
                if (j % 2 == 0)
                {
                    printf("+");
                }

                else
                {
                    printf("---");
                }
            }

            else
            {
                if (j % 2 == 0)
                {
                    printf("|");
                }

                else
                {
                    printf(" %c ", board[n]);
                    n++;
                }
            }
        }

        printf("\n");
    }
}

int clear(int h, int w)
{
    printf("\033[0;0H");

    for (int i = 0; i < w; i++)
    {
        for (int j = 0; j < h; j++)
        {
            printf(" ");
        }
    }

    printf("\033[0;0H");
}

int freeSpace()
{
    for (int i = 0; i < sizeof(board); i++)
    {
        if (board[i] == ' ')
        {
            return true;
        }
    }
}

int player()
{
    printf("Player %d: ", player_num);

    if (freeSpace())
    {
        scanf("%d", &player_input);
    }

    while (board[player_input - 1] != ' ')
    {
        printf("\033[1APlayer %d: Not empty, try again: ", player_num);
        scanf("%d", &player_input);
    }

    board[player_input - 1] = symbol;
}

int checkLine(int x, int y, int z)
{
    if ((board[x] == symbol) && (board[x] == board[y]) && (board[y] == board[z]))
    {
        printf("Player %d wins!\n", player_num);
        exit(0);
    }
}

int winner()
{
    checkLine(0, 1, 2); //first horizontal

    checkLine(3, 4, 5); //second horizontal

    checkLine(6, 7, 8); //third horizontal

    checkLine(0, 3, 6); //first vertical

    checkLine(1, 4, 7); //second vertical

    checkLine(2, 5, 8); //third vertical

    checkLine(0, 4, 8); //first diagonal

    checkLine(2, 4, 6); //second diagonal
}
\$\endgroup\$
  • \$\begingroup\$ Please include the header files, this program currently doesn't compile because the headers are missing. \$\endgroup\$ – pacmaninbw Oct 31 '19 at 14:33
  • 1
    \$\begingroup\$ Okay, it's edited now, \$\endgroup\$ – AlexaN Oct 31 '19 at 14:36
2
\$\begingroup\$

Compiler Options and Warning Checking

It might be a good idea to use the -wall compiler switch. It will provide warning messages that may indicate possible logic errors in some cases. When compiled with -wall this program yields the following warning messages:

D:\ProjectsNfwsi\CodeReview\tictactoev1\main.c(89) : warning C4716: 'printBoard': must return a value
D:\ProjectsNfwsi\CodeReview\tictactoev1\main.c(104) : warning C4716: 'clear': must return a value
D:\ProjectsNfwsi\CodeReview\tictactoev1\main.c(115) : warning C4715: 'freeSpace': not all control paths return a value
D:\ProjectsNfwsi\CodeReview\tictactoev1\main.c(133) : warning C4716: 'player': must return a value
D:\ProjectsNfwsi\CodeReview\tictactoev1\main.c(142) : warning C4715: 'checkLine': not all control paths return a value
D:\ProjectsNfwsi\CodeReview\tictactoev1\main.c(161) : warning C4716: 'winner': must return a value

In some cases the functions should be declared void rather than int, in other cases there might be a bug when not all control paths return a value, this is certainly true in freeSpace.

Global Variables

Global variables make programs very, very difficult to write, read, debug and maintain. In the C programming language they can cause modules (other .c files) not link if the global variable is declared in multiple modules. Due to the nature of global variables, it is sometimes very difficult to find where they are changed in order to remove a bug.

There is a discussion about global variables in this stackoverflow question

It is much better to pass a variables into functions where they are needed.

Avoid Using exit()

For a number of reasons the use of the exit() function should be avoided. The exit() function should be used when there is a non-recoverable error in a program. Since it is not being used for this purpose in this program it should definitely be avoided. It would be much better if the functions checkLine() and winner() returned values that indicated status.

\$\endgroup\$
  • \$\begingroup\$ Okay, I'll correct all what you said, but what are other ways to close the program when the game is finished? Also, how good is my code in terms of clarity, readibility, and looking clean and understandable in general? \$\endgroup\$ – AlexaN Oct 31 '19 at 15:45
  • 1
    \$\begingroup\$ @AlexaN The code is readable, the variable names are pretty good or I would have mentioned it in the review. About closing the program, it should always be done from main, that is what return 0 is about. Test the return value from winner() in main and exit if true. \$\endgroup\$ – pacmaninbw Oct 31 '19 at 15:49
  • \$\begingroup\$ Ok, thank you! XD \$\endgroup\$ – AlexaN Oct 31 '19 at 15:58
  • \$\begingroup\$ You should stick to one naming convention; some of your variables use snake_case (player_input) while your methods use camelCase (checkLine). You can take a look at this answer (stackoverflow.com/a/1722518) for a more detailed explanation of C's naming conventions. \$\endgroup\$ – cliesens Nov 1 '19 at 17:52
  • \$\begingroup\$ @cliesens Okay, I'll fix it right away. \$\endgroup\$ – AlexaN Nov 2 '19 at 13:36
2
\$\begingroup\$

1. Determination the length of an array

Look at the marked line:

int freeSpace()
{
    for (int i = 0; i < sizeof(board); i++) // <----
    {
        if (board[i] == ' ')
        {
            return true;
        }
    }
}

This code works because board has type char [9]. For this reason sizeof(board) equals to the number of elements in board1. But this code may fails, in case if you change the type of board.

The correct way to determine the size of an array is

sizeof(board) / sizeof(board[0])

It would be better to define a macro:

#define ARRAY_LENGTH(a) (sizeof(a) / sizeof((a)[0]))

2. Declaration of a function that takes no parameters

The correct way to declare a function without any parameters is

int printBoard(void);

instead of

int printBoard();

Please, for more details, take a look at this question on SO.


1 Since sizeof(char) always equals to 1.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the explanation, I'll correct it. \$\endgroup\$ – AlexaN Nov 1 '19 at 17:20

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.