6
\$\begingroup\$

I recently started learning C, though I have some experience in other languages. I recently wrote a TicTacToe AI using the Minimax Algorithm in C. I would like to know how I could write better C.

#include <stdio.h>

#define X -1
#define O -2
#define MAX_SIZE 50 //This is the Max size of the Input Buffer

int turn = O; 

int board[] = {1, 2, 3, 4, 5, 6, 7, 8, 9};

int variations = 0; //To keep track of no. of variations the ai has seen

void copyBoard(int from[], int to[]){
    for(int i = 0; i < 9; i++){
        to[i] = from[i];
    }
}

//gets line WITHOUT \n 
int getl(char s[], int lim){
    int c, i;

    for(i = 0; i < lim-1 && (c = getchar()) != EOF && c != '\n'; i++)
        s[i] = c;
    
    s[i] = '\0';
    return i;
}

//Converts char[] to int
int bufferToNum(char buffer[]){
    int n = 0;
    for(int i = 0; buffer[i] != '\0'; i++){
        n = 10 * n + buffer[i] - '0';
    }
    return n;
}

//converts the board numbers to char to display
char boardToChar(int i){
    int a = board[i];
    if (a == X){        
        return 'X';
    } else if (a == O){
        return 'O';
    } else {    
       return a + '0';
    }    
}

//prints board
void printBoard(){
    printf("=============\n| %c | %c | %c |\n-------------\n| %c | %c | %c |\n-------------\n| %c | %c | %c |\n=============\n", boardToChar(0), boardToChar(1), boardToChar(2), boardToChar(3), boardToChar(4), boardToChar(5), boardToChar(6), boardToChar(7), boardToChar(8));    
}

//alternates turn
void alternateTurn(){
    if (turn == O){
        turn = X;    
    } else if (turn == X){
        turn = O;
    }
}

//returns 1 if draw, return 0 if not a draw
int drawCheck(int l_board[]){
    for(int i = 0; i < 9; i++){
        if (l_board[i] == i+1){
            return 0;
        }
    }
    return 1;
}

//returns X if X won and O if O one and 0 if nobody winning
int winCheck(int l_board[]){
    //Rows
    for (int i = 0; i < 3; i++){
        if (l_board[3*i] == l_board[3*i + 1] && l_board[3*i + 1] == l_board[3*i + 2]){
            return l_board[3*i];
        }
    }

    //Columns
    for (int j = 0; j < 3; j++){
        if (l_board[j] == l_board[3 + j] && l_board[3 + j] == l_board[6 + j]){
            return l_board[j];
        }
    }

    //Diagonal Top Left to Bottom Right
    if (l_board[0] == l_board[4] && l_board[0] == l_board[8]){
        return l_board[0];
    }

    //Diagonal Top Right to bottom Left
    if (l_board[2] == l_board[4] && l_board[2] == l_board[6]){
        return l_board[2];
    }

    return 0;
}

//1 if nothing is ther and 0 if something was already ther
int putInBoard(int l_board[], int pos, int newVal){
    if (l_board[pos] == pos+1){
        l_board[pos] = newVal;
        return 1;
    } else
    {
        return 0;
    }    
}

//X if X win, O if O win, 0 if draw, 1 if nothing
int gameState(int l_board[]){
    int wc = winCheck(l_board);
    if (wc == X){
        return X;
    } else if(wc == O){
        return O;
    } else {
        if (drawCheck(l_board)){
            return 0;
        }
    }
    return 1;
}

void legalMoves(int l_board[], int output[]){    
    for(int i = 0; i < 9; i++){
        if (l_board[i] == i+1){
            output[i] = 1;
        } else {
            output[i] = 0;
        }
    }
}

int max(int a, int b){
    return a>b ? a : b;
}

int min(int a, int b){
    return a<b ? a : b;
}

//X is ai
int minimax(int l_board[], int depth, int maximising){
    int gs = gameState(l_board);
    variations++;
    if (gs == X){
        return 10;
    } else if (gs == O){
        return -10;
    } else if (gs == 0){
        return 0;
    }
    if (depth == 0){
        return 0;
    }
    if (maximising){
        //Its AI's Turn so it has to maximise
        int val = -100;
        int legalMovesArr[9];
        legalMoves(l_board, legalMovesArr);
        for (int i = 0; i < 9; i++){
            if (legalMovesArr[i]){
                int tempBoard[9];
                copyBoard(l_board, tempBoard);
                putInBoard(tempBoard, i, X);
                val = max(minimax(tempBoard, depth-1, 0), val);
            }
        }
        return val;
    } else {        
        int val = 100;
        int legalMovesArr[9];
        legalMoves(l_board, legalMovesArr);
        for (int i = 0; i < 9; i++){
            if (legalMovesArr[i]){
                int tempBoard[9];
                copyBoard(l_board, tempBoard);
                putInBoard(tempBoard, i, O);
                val = min(minimax(tempBoard, depth-1, 1), val);
            }
        }
        return val;
    }
}

int ai(int l_board[], int depth){
    int legalMovesArr[9];
    legalMoves(board, legalMovesArr);
    int val = -100;    
    int best_move = 0;
    for (int i = 0; i < 9; i++){
        if (legalMovesArr[i]){
            int tempBoard[9];
            copyBoard(l_board, tempBoard);
            putInBoard(tempBoard, i, X);
            int temp = minimax(tempBoard, depth-1, 0);
            if (val <= temp){
                val = temp;
                best_move = i;
            }            
        }
    }
    return best_move;
}

int main(){
    printBoard();
    int gameOn = 0;
    char buffer[MAX_SIZE];
    while(!gameOn){
        if (turn == O){
            printf("%c's turn: ", turn == X ? 'X' : 'O');
            getl(buffer, MAX_SIZE);
            int num = bufferToNum(buffer);        
            while (num <= 0 || num > 9){
                printf("Please enter an integer between 1 and 9: ");
                getl(buffer, MAX_SIZE);
                num = bufferToNum(buffer);            
            }
            if (putInBoard(board, num-1, turn)){
                ;
            } else {            
                while(!putInBoard(board, num-1, turn)){
                    printf("Something already exists, Please enter a new number: ");
                    getl(buffer, MAX_SIZE);
                    num = bufferToNum(buffer);
                }
            }
        } else {
            putInBoard(board, ai(board, 8), X);
            printf("Calculated %d variations\n", variations);
            variations = 0;
        }
        
        printBoard();                
        alternateTurn();
        int gs = gameState(board);
        if (gs == X){
            printf("X won!");
            return 0;
        } else if (gs == O){
            printf("O won!");
            return 0;
        } else if (gs == 0){
            printf("Draw!");
            return 0;
        }           
    }
    
    return 0;
}
\$\endgroup\$
9
\$\begingroup\$

General Observations

I see a lot of good programming practices followed here already, keep up the good work.

Most of the functions are small and follow the Single Responsibility Principle.

Use Library Functions When You Can

The function getl() can be implemented using 2 standard C library functions, fgets and strrchr.

The fgets() function inputs an entire line of characters at a time, rather than using character input, and the strrchr() will allow you to find the \n character and replace it.

DRY Code

There is a programming principle called the Don't Repeat Yourself Principle sometimes referred to as DRY code. If you find yourself repeating the same code multiple times it is better to encapsulate it in a function. If it is possible to loop through the code that can reduce repetition as well.

The repetition occurs in the function int minimax(int l_board[], int depth, int maximising) :

    if (maximising) {
        //Its AI's Turn so it has to maximise
        int val = -100;
        int legalMovesArr[9];
        legalMoves(l_board, legalMovesArr);
        for (int i = 0; i < 9; i++) {
            if (legalMovesArr[i]) {
                int tempBoard[9];
                copyBoard(l_board, tempBoard);
                putInBoard(tempBoard, i, X);
                val = max(minimax(tempBoard, depth - 1, 0), val);
            }
        }
        return val;
    }
    else {
        int val = 100;
        int legalMovesArr[9];
        legalMoves(l_board, legalMovesArr);
        for (int i = 0; i < 9; i++) {
            if (legalMovesArr[i]) {
                int tempBoard[9];
                copyBoard(l_board, tempBoard);
                putInBoard(tempBoard, i, O);
                val = min(minimax(tempBoard, depth - 1, 1), val);
            }
        }

This repeating code could be it's own function.

The repeating code also makes the function too complex.

Complexity

The complexity of the function minimax() was mentioned above, the function main() is also 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.

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.

The entire while (!gameOn) loop should be within its own function.

Avoid Global Variables

It is very difficult to read, write, debug and maintain programs that use global variables. Global variables can be modified by any function within the program and therefore require each function to be examined before making changes in the code. In C and C++ global variables impact the namespace and they can cause linking errors if they are defined in multiple files. The answers in this stackoverflow question provide a fuller explanation.

Update Answer to comment

void game_loops()
{
    if (turn == O) {
        printf("%c's turn: ", turn == X ? 'X' : 'O');
        char buffer[MAX_SIZE];
        getl(buffer, MAX_SIZE);
        int num = bufferToNum(buffer);
        while (num <= 0 || num > 9) {
            printf("Please enter an integer between 1 and 9: ");
            getl(buffer, MAX_SIZE);
            num = bufferToNum(buffer);
        }
        if (putInBoard(board, num - 1, turn)) {
            ;
        }
        else {
            while (!putInBoard(board, num - 1, turn)) {
                printf("Something already exists, Please enter a new number: ");
                getl(buffer, MAX_SIZE);
                num = bufferToNum(buffer);
            }
        }
    }
    else {
        putInBoard(board, ai(board, 8), X);
        printf("Calculated %d variations\n", variations);
        variations = 0;
    }

    printBoard();
    alternateTurn();
}

int main() {
    printBoard();

    int gs = 1;
    while (gs == 1) {
        game_loops();
        gs = gameState(board);
    }

    switch (gs)
    {
    case X:
        printf("X won!");
        break;

    case O:
        printf("O won!");
        break;

    default:
        printf("Draw!");
        break;
    }

    return 0;
}
\$\endgroup\$
4
  • \$\begingroup\$ First of all, thanks for your answer. I have few comments and questions though. 1)I understand your point of getl and I would use the standard library functions in actual code, but I wanted to do low-level stuff and implement my own functions for these tasks. 2) How would copying the whole main() code to a function help in readability? 3)I think I had minimal global variables, what variables should not have been in the global scope? Thank you for your time. \$\endgroup\$ – kprogrammer Aug 18 '20 at 17:38
  • 4
    \$\begingroup\$ The minimal and desired number of global variables is zero, nothing should be done by side affects. This program is very easy to write without global variables. I've updated the answer to show how it readable the loop would be, but every removal of indentation helps readability. You don't want to use character by character input when you can use buffered input instead for performance reasons. \$\endgroup\$ – pacmaninbw Aug 18 '20 at 18:10
  • \$\begingroup\$ I understand, thank you! \$\endgroup\$ – kprogrammer Aug 19 '20 at 2:22
  • 1
    \$\begingroup\$ "getl() can be implemented using 2 standard C library functions" --> OP's getl() has an obscure advantage: it returns the correct number of characters read when one of them is surprisingly a null character. fgets()/strrchr() needs more to support that rare need. Also the fgets() approach needs a +1 size buffer. \$\endgroup\$ – chux - Reinstate Monica Aug 19 '20 at 5:48
4
\$\begingroup\$

Small review

Break up that long line of code.

//prints board
void printBoard(){
  printf("=============\n| %c | %c | %c |\n-------------\n| %c | %c | %c |\n-------------\n| %c | %c | %c |\n=============\n", boardToChar(0), boardToChar(1), boardToChar(2), boardToChar(3), boardToChar(4), boardToChar(5), boardToChar(6), boardToChar(7), boardToChar(8));    
}

Perhaps as

//prints board
void printBoard() {
  printf("=============\n"
      "| %c | %c | %c |\n"
      "-------------\n"
      "| %c | %c | %c |\n"
      "-------------\n"
      "| %c | %c | %c |\n"
      "=============\n", //
      boardToChar(0), boardToChar(1), boardToChar(2), //
      boardToChar(3), boardToChar(4), boardToChar(5), // 
      boardToChar(6), boardToChar(7), boardToChar(8));    
}

For me, the // at the end of 3 lines prevent auto formatting of those lines into 1 or 2 lines.

\$\endgroup\$
3
  • \$\begingroup\$ Yes, that looks much better. I remember from K&R that we can do "hello" " world" but I didn't think of it while coding. Thanks! \$\endgroup\$ – kprogrammer Aug 19 '20 at 7:09
  • \$\begingroup\$ Wouldn't a printf for each line be cleaner and easier to maintain? \$\endgroup\$ – alx Aug 24 '20 at 10:09
  • \$\begingroup\$ @CacahueteFrito Versu OP's code, "printf for each line" sounds like an interesting alternative to this answer's re-format only approach. Perhaps post as an answer? For me, 6.01 vs. half-a-dozen of the other - a difference, but not much. \$\endgroup\$ – chux - Reinstate Monica Aug 24 '20 at 14:19

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.