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

Single player Tic-Tac-Toe game

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

Problem

I have made an attempt to make a single player mode for the Tic-Tac-Toe game. I am not sure if this is the right way to do it and wanted a second opinion.

// keeps track of all the valid positions.
bool validPos[10] = {false, true, true, true, true, true, true, true, true, true};
char user = 'O', computer = 'X';

int main(void)
{
    // A two-dimensional array depicting the game.
    char game[3][3] = {'1', '2', '3', '4', '5', '6', '7', '8', '9'};
    char choice;

    // Asks user for a valid input.
    do
    {
        printf("X or O ?\n");
        scanf(" %c", &choice);

    }while(choice != 'X' && choice != 'O' && choice != 'x' && choice != 'o');

    // assigns X or O to the player.
    if(choice == 'x' || choice == 'X')
    {
        user = 'X';
        computer = 'O';
    }

    display(game);

    printf("You - %c\n", user);
    printf("Computer - %c\n", computer);

    startGame(game, user);
}


Function startGame

```
void startGame(char game[3][3], const char turn)
{
int i, j;
int pos;

srand((unsigned)time(NULL));
if(turn == user)
{
// asks user for a valid input.
do
{
printf("Your turn : ");
scanf("%d", &pos);

}while((pos > 9 || pos 9 || pos < 1) || !validPos[pos]);
pos = calcMove(game, pos);
}

// makes the position chosen invalid.
validPos[pos] = false;

// Assigns 'X' or 'O' to the chosen position.
for( i = 0; i < 3; i++)
for( j = 0; j < 3; j++)
if(game[i][j] - '0' == pos)
game[i][j] = turn;

// prints the game onto the screen.
display(game);

// checks if the player or computer has won or not.
if(gameStatus(game))
{
if(turn == user)
printf("You Win !\n");

else
printf("Computer Wins !\n");

return;
}

// checks if there are any further moves possible.
// also acts as the base case for the game to end.
if(isComplete(game

Solution

Here are a number of things that may help you improve your code.

Match initializaton with declaration

The variable game is currently declared and initialized like this:

char game[3][3] = {'1', '2', '3', '4', '5', '6', '7', '8', '9'};


But those don't really match. I'd recommend instead to have them match by changing that line to this:

char game[3][3] = {{'1', '2', '3'}, {'4', '5', '6'}, {'7', '8', '9'}};


Eliminate global variables where practical

Having routines dependent on global variables makes it that much more difficult to understand the logic and introduces many opportunities for error. Eliminating global variables where practical is always a good idea, whether programming for desktop machines or for embedded systems. In this code, I would recommend that all of the game state variables, including the board, the current turn and the tokens used could all be wrapped inside one structure and then a pointer to that structure could be passed to each of the relevant functions. This makes it much easier to understand and much easier to maintain.

Eliminate redundant variables

The validPos array is not really needed given that the game array is passed to each function. A position is "valid" in the sense of this game if it's not already occupied, so instead of this:

if(game[i][0] == game[i][1] && validPos[game[i][2] - '0'])


You could write this:

if(game[i][0] == game[i][1] && isdigit(game[i][2]))


Don't reseed the random number generator more than once

The program currently calls srand before every turn. This is really neither necessary nor advisable. Instead, just call it once when the program begins and then continue to use rand() to get random numbers.

Understand how rand() works

The rand() function returns a number between 0 and RAND_MAX. Just for simplicity, let's say that RAND_MAX is 15 (it's actually much larger than that). A random number generator (or psuedo-random number generator) should generate each possible number with equal frequency, so we'd have 0 occur one sixteenth of the time, 1 occur one sixteenth of the time, etc. So far so good, but let's see what happens when we use a construct like this:

pos = rand() % 10;


Here is the probability table:

rand    pos     probability
0       0           1/16
1       1           1/16
2       2           1/16
3       3           1/16
4       4           1/16
5       5           1/16
6       6           1/16
7       7           1/16
8       8           1/16
9       9           1/16
10      0           1/16
11      1           1/16
12      2           1/16
13      3           1/16
14      4           1/16
15      5           1/16


The resulting probability for pos, then is this:

pos     probability
0           2/16
1           2/16
2           2/16
3           2/16
4           2/16
5           2/16
6           1/16
7           1/16
8           1/16
9           1/16


Because 10 doesn't divide evenly into RAND_MAX, the numbers at the lower end of the range are statistically more likely to be selected than those at the end of the range. For a simple game like this, it may not matter much, but it's an important concept to understand. To make it so that you have an even distribution, you could do this:

int pos = rand() / (RAND_MAX / N + 1);


Where N is defined as the upper limit of your range (10 in this case).

Separate input, output and calculation

To the degree practical it's usually good practice to separate input, output and calculation for programs like this. By putting them in separate functions, it isolates the particular I/O for your platform (which is likely to be unique to that platform or operating system) from the logic of your program (which does not depend on the underlying OS). The prompts for the user and the getting and validation of the user's move could be separated from the main game logic, maintaining a clean separation between I/O and game logic.

Use better naming

The function isComplete is not bad because it makes it obvious that a value of true returned would mean that the game is complete. (Although I might prefer isFull to distinguish between that state and the state in which one player has won.) The gameStatus function, however, does not have such an easy interpretation. So instead, I'd probably name that function isWon.

Don't use recursion when not needed

The recursive calls to startGame are not really needed. In this case, there's no great harm, but in cases in which there were many more calls, it could blow up the stack and cause your program to crash. A better way to do that is to isolate just the game logic so that instead of startGame we could name the function playGame and it might look like this:

```
bool playGame (void) {
char game[3][3] = {{'1', '2', '3'}, {'4', '5', '6'}, {'7', '8', '9'}};
bool humanTurn = true;
while (!isWon(game) && !isFull(game))
{
disp

Code Snippets

char game[3][3] = {'1', '2', '3', '4', '5', '6', '7', '8', '9'};
char game[3][3] = {{'1', '2', '3'}, {'4', '5', '6'}, {'7', '8', '9'}};
if(game[i][0] == game[i][1] && validPos[game[i][2] - '0'])
if(game[i][0] == game[i][1] && isdigit(game[i][2]))
pos = rand() % 10;

Context

StackExchange Code Review Q#154568, answer score: 4

Revisions (0)

No revisions yet.