4
\$\begingroup\$

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;

}
\$\endgroup\$
1
5
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks! I don't really have a very profound reason, it's just that I think the "print" makes printf clearer than puts. I like the principle of least astonishment, and my impression is that printf is more common. I base that only on code I've read and not on any research, so I may well be biased. \$\endgroup\$ – Lstor Jun 16 '13 at 20:53
  • \$\begingroup\$ Thanks for the review. Regarding puts, my friend who's a software developer said to use it if you're just printing a string because it doesn't have to analyze the string format that printf does. As far as I know, it probably isn't gonna make much difference for speed, other than its 2 letters shorter to type. I've just always had the habit of doing that. \$\endgroup\$ – Atom Jun 20 '13 at 3:00
  • 1
    \$\begingroup\$ It's very typical of C-programmers to think like that, but I recommend preferring readability over performance except in the critical path :) By the way, if you feel that the answer is satisfactory, feel free to click accept answer. If you like another answer better, accept it instead. \$\endgroup\$ – Lstor Jun 20 '13 at 3:04
3
\$\begingroup\$

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]
\$\endgroup\$
1
  • \$\begingroup\$ +1. Many valid points that I had missed. Regarding using sizeof with memset: I wholly agree. When doing so, however, it is very important not to do it through a function. Arrays are demoted to pointers when passed to functions, and the size will in that case be the same as sizeof(char*). \$\endgroup\$ – Lstor Jun 17 '13 at 0:07

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.