HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Connect Four game

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
gamefourconnect

Problem

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  
#include 
#include 
#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  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 = 3 && checkFour(board, idx, idx + DIAG_RGT, idx + DIAG_RGT * 2, idx + DIAG_RGT * 3)){
            return 1;
         }
         count++;
      }
      count = 0;
   }
   return 0;

}

Solution

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.

Code Snippets

void clearScreen() {
#ifdef WIN32
    system("cls");
#else
    system("clear");
#endif // WIN32
}
for (int row = 0; row < BOARD_ROWS; ++row)
}
    int diagonalCheck(char *board){
return 0;
    }

    int diagonalCheck(char *board) {

Context

StackExchange Code Review Q#27446, answer score: 6

Revisions (0)

No revisions yet.