Connect Four game

Question

I wrote this a couple months ago and just wanted to get some feedback on it. In the printBoard() function, you see I commented out the system function. I've read this is considered bad practice. Does anyone know of any alternative to clearing the terminal window? I thought it'd be a nice touch to clear the window instead of just having the updated board appear below the previous.

#include <stdio.h> 
#include <string.h>
#include <stdlib.h>
#define BOARD_ROWS 6
#define BOARD_COLS 7

void printBoard(char *board);
int takeTurn(char *board, int player, const char*);
int checkWin(char *board);
int checkFour(char *board, int, int, int, int);
int horizontalCheck(char *board);
int verticalCheck(char *board);
int diagonalCheck(char *board);

int main(int argc, char *argv[]){
   const char *PIECES = "XO";
   char board[BOARD_ROWS * BOARD_COLS];
   memset(board, ' ', BOARD_ROWS * BOARD_COLS);

   int turn, done = 0;

   for(turn = 0; turn < BOARD_ROWS * BOARD_COLS && !done; turn++){
      printBoard(board);   
      while(!takeTurn(board, turn % 2, PIECES)){
         printBoard(board);   
         puts("**Column full!**\n");
      }
      done = checkWin(board);
   } 
   printBoard(board);

   if(turn == BOARD_ROWS * BOARD_COLS && !done){
      puts("It's a tie!");
   } else {
      turn--;
      printf("Player %d (%c) wins!\n", turn % 2 + 1, PIECES[turn % 2]);
   }

   return 0;

}
void printBoard(char *board){
   int row, col;

   //system("clear");
   puts("\n    ****Connect Four****\n");
   for(row = 0; row < BOARD_ROWS; row++){
      for(col = 0; col < BOARD_COLS; col++){
         printf("| %c ",  board[BOARD_COLS * row + col]);
      }
      puts("|");
      puts("-----------------------------");

   }
   puts("  1   2   3   4   5   6   7\n");

}
int takeTurn(char *board, int player, const char *PIECES){
   int row, col = 0;
   printf("Player %d (%c):\nEnter number coordinate: ", player + 1, PIECES[player]);

   while(1){ 
      if(1 != scanf("%d", &col) || col < 1 || col > 7 ){
         while(getchar() != '\n');
         puts("Number out of bounds! Try again.");
      } else { 
         break;
      }
   }
   col--;

   for(row = BOARD_ROWS - 1; row >= 0; row--){
      if(board[BOARD_COLS * row + col] == ' '){
         board[BOARD_COLS * row + col] = PIECES[player];
         return 1;
      }
   }
   return 0;

}
int checkWin(char *board){
    return (horizontalCheck(board) || verticalCheck(board) || diagonalCheck(board));

}
int checkFour(char *board, int a, int b, int c, int d){
    return (board[a] == board[b] && board[b] == board[c] && board[c] == board[d] && board[a] != ' ');

}
int horizontalCheck(char *board){
    int row, col, idx;
    const int WIDTH = 1;

    for(row = 0; row < BOARD_ROWS; row++){
       for(col = 0; col < BOARD_COLS - 3; col++){
          idx = BOARD_COLS * row + col;
          if(checkFour(board, idx, idx + WIDTH, idx + WIDTH * 2, idx + WIDTH * 3)){
             return 1;
          }
       }
    }
    return 0;

}
int verticalCheck(char *board){
    int row, col, idx;
    const int HEIGHT = 7;

    for(row = 0; row < BOARD_ROWS - 3; row++){
       for(col = 0; col < BOARD_COLS; col++){
          idx = BOARD_COLS * row + col;
          if(checkFour(board, idx, idx + HEIGHT, idx + HEIGHT * 2, idx + HEIGHT * 3)){
              return 1;
          }
       }
    }
    return 0;

}
int diagonalCheck(char *board){
   int row, col, idx, count = 0;
   const int DIAG_RGT = 6, DIAG_LFT = 8;

   for(row = 0; row < BOARD_ROWS - 3; row++){
      for(col = 0; col < BOARD_COLS; col++){
         idx = BOARD_COLS * row + col;
         if(count <= 3 && checkFour(board, idx, idx + DIAG_LFT, idx + DIAG_LFT * 2, idx + DIAG_LFT * 3) || count >= 3 && checkFour(board, idx, idx + DIAG_RGT, idx + DIAG_RGT * 2, idx + DIAG_RGT * 3)){
            return 1;
         }
         count++;
      }
      count = 0;
   }
   return 0;

}

Answer 1

Some comments, in no particular order:

1: For applications like this, I think it is perfectly legitimate to use system(). It's not exactly elegant, but in this particular case I think it's fine. You could do:

void clearScreen() {
#ifdef WIN32
    system("cls");
#else
    system("clear");
#endif // WIN32
}

2: Consider using the C99 or C11 standards. They allow a couple of things that make the code easier to read and maintain. For example, substitute your #defines with const int BOARD_ROWS = 6. This helps debugging (because you see BOARD_ROWS and not 6 in the compiler and debugger output), adds scope to the variables and generally avoids most of the problems with #defines.

C99 and C11 don't require that variable definitions appear at the beginning of their scope. This means you can keep variables closer to their relevant context. The most useful example for this is for loops. In C99 and C11, you can write

 for (int row = 0; row < BOARD_ROWS; ++row)

C99 also introduced a bool type (which is actually just a typedef for int). This makes the code more readable. Usage example: bool done = false; while (!done) { ... done = true; }. The bool type is of course available in C11 as well.

3: What is the value of row after int row, col = 0;? It's unspecified, so row can have any value. If you mean to initialize both variables, which you should, you need to write int row = 0, col = 0;.

(There's a similar gotcha in C. What's the type of p2 in this snippet: int *p1, p2;? It's a plain int, only p1 is an int*. To make both variables pointers, write int *p1, *p2.)

4: Consider using printf consistently instead of puts.

5: Consider preferring pre-increment operators. row++ means "increment row, then return a copy of what it looked like before incrementing". ++row means "increment row", which is what you mean. It's not a big difference in this case, and the compiler can optimize away the extra copy, but I like the habit of writing exactly what you mean. If you ever start writing C++-code with user-defined increment operators, this can actually be important.

6: Try to always initialize your variables, rather than set their value at a later time. It's faster, cleaner and safer. Note that static and global variables are guaranteed to be zero-initialized when the program starts, but it doesn't hurt to be explicit.

7: Full variable names are easier to read, and there's no good reason not to type them out. Prefer index over idx, DIAGONAL_RIGHT over DIAG_RGT and so on.

8: Personally, I prefer for (;;) over while (1). The former is commonly read out loud as "forever".

9: Consider adding documentation and comments to your code. For example Doxygen comments before each function.

10: Code that is not self-explanatory should be factored out into separate functions with meaningful names. Consider your while(getchar() != '\n');. Putting this into a function called either flushInputBuffer() or waitForEnter() conveys intent and what is going on much better than the original code.

11: Consider changing your whitespace to increase readability. Instead of return 0;

    }
    int diagonalCheck(char *board){

write

    return 0;
    }

    int diagonalCheck(char *board) {

to emphasize that the closing bracket belongs to the topmost function, not the bottom one. Also consider whitespace before brackets, for example. This is a matter of preference, the most important thing is that you're consistent and that the code is readable.

12: Break up long lines into more, shorter lines. This is a tremendous help for readability, and especially helps when doing source control merges, debugging in a debugger and other cases where your window width is limited. It's common to limit lines to 80-100 characters.

13: Consider sorting #includes in alphabetical order.

14: Consider using unit tests.

15: There are several things you are doing that are good. For example using symbolic rather than literal constants, i.e. const int DIAG_RGT = 6, DIAG_LFT = 8;. Your program is also fairly well divided into functions, and most variable names are reasonably named. Keep doing these things.

Answer 2

Here are some extra points not covered by the excellent review from @Lstor

You can avoid the need for prototypes by putting main at the end. This might seem odd but it is quite common practice. Also, your functions could all be static, as they are used only in this file. For single-file programs this is of no significance, but for large programs it is best practice as it avoids polluting the namespace and may make optimisation possible.

---

This:

char board[BOARD_ROWS * BOARD_COLS];
memset(board, ' ', BOARD_ROWS * BOARD_COLS);

is better written with sizeof

char board[BOARD_ROWS * BOARD_COLS];
memset(board, ' ', sizeof board);

This guarantees that the array is cleared even if you were to change the size of board and forgot to change the memset line.

---

As there are only two ways of leaving the for-loop in main, namely when (turn >= BOARD_ROWS * BOARD_COL) and when done is set, this line following the loop:

if(turn == BOARD_ROWS * BOARD_COLS && !done){

is the same as just testing !done

---

Your input loop in takeTurn is unusual. I would usually use fgets rather than scanf (using scanf results in you having to purge the input on false data, as you discovered, because scanf doesn't remove the false data from the input stream. Also your loop doesn't check for the input stream being closed):

int col = 0;
char buf[10];
while (fgets(buf, sizeof buf, stdin) != NULL) {
    col = (int) strtol(buf, 0, 0);
    if (col >= 1 && col <= 7 ) {
        break;
    }
    puts("Number out of bounds! Try again.");
}
if (col == 0) { // user entered ctrl-d to end input
    exit(1);
}

---

The fact that you always call checkBoard with the 4th and 5th parameters multiplied by 2 and 3 respectively implies to me that the function might be better doing the multiplications.

---

Your long expression:

if(count <= 3 &&
   checkFour(board, idx, idx + DIAG_LFT, idx + DIAG_LFT * 2, idx + DIAG_LFT * 3) ||
   count >= 3 &&
   checkFour(board, idx, idx + DIAG_RGT, idx + DIAG_RGT * 2, idx + DIAG_RGT * 3)){
   return 1;
}

needs some brackets. The compiler gives me this warning:

 warning: '&&' within '||' [-Wlogical-op-parentheses]