11
\$\begingroup\$

Please help me reduce its complexity and maybe optimize it a little bit more.

#include <stdio.h>
#include <string.h>
#include <time.h>

#define size 3
void Clear_board(char (*)[size]);
void Player_turn(char (*)[size],char);
void PC_turn(char (*)[size],char,char,int*);
void Print_board(char (*)[size]);
void Scan_corners(char (*)[size],char,char);
int Check_board(char (*)[size],char,int*);
int Action(char (*)[size],char,char,int);
int Scan_diag(char (*)[size],char,char,int);
int Scan_rowscols(char (*)[size],char,char,int);




int main()
{
//-------------------------
char xo[size][size]={'1','2','3','4','5','6','7','8','9'};
char Fig_PC='X';
char Fig_Player='O';
int PC_move=0;
//-------------------------

printf("Welcom to Tic Tac Toe \n");
Print_board(xo);
Clear_board(xo);

//PC plays first and increments PC_move
PC_turn(xo,Fig_PC,Fig_Player,&PC_move);


//While nobody won, or PC reaches max move game is on
while ((Check_board(xo,Fig_PC,&PC_move)))
{
Player_turn(xo,Fig_Player);
PC_turn(xo,Fig_PC,Fig_Player,&PC_move);
}
return 0;
}


void PC_turn(char (*xo)[size],char Fig_PC,char Fig_Player,int *PC_move)
{
printf("Computer's Turn:\n");

/*
First try to win by completing an X X X
Second priority is to defend
Lastly try to take postion of corners
*/

if (!Action(xo,Fig_PC,Fig_Player,1))
    {
        if (!Action(xo,Fig_Player,Fig_PC,0))
        {
            Scan_corners(xo,Fig_PC,Fig_Player);
        }
    }

    Print_board(xo);
    ++*PC_move;
}


void Print_board(char (*xo)[size])
{
int i=0;

        printf("\n\n   |   |    \n");
        printf(" %c | %c | %c\n", xo[2][0], xo[2][1], xo[2][2]);
        printf("___|___|___ \n");
        printf("   |   |    \n");
        printf(" %c | %c | %c\n", xo[1][0], xo[1][1], xo[1][2]);
        printf("___|___|___ \n");
        printf("   |   |    \n");
        printf(" %c | %c | %c\n", xo[0][0], xo[0][1], xo[0][2]);
        printf("   |   |    \n\n\n");

    usleep(1000000);
}


void Clear_board(char (*xo)[size])
{
    int i=0;
    int j=0;

    for (i=0;i<size;++i)
    {
        for (j=0;j<size;++j)
        {
        xo[i][j]=' ';
        }
    }
}



void Player_turn(char (*xo)[size], char Fig_Player)
{
// Put O where player wishes, unless illegal entry or taken cell.

int choice=0;
printf("player's Turn:\n");
printf("choose %c position\n", Fig_Player);

while ((choice<1) || (choice>9))
{
    scanf("%d",&choice);
    if ((choice>0) && (choice<10) && (xo[(choice-1)/size][(choice-1)%size]==' '))
    {
    xo[(choice-1)/size][(choice-1)%size]=Fig_Player;
    Print_board(xo);
    }
    else
    {
    printf("Please try again\n");
    }
}
}



int Action(char (*xo)[size],char Fig_PC,char Fig_Player, int Action)
{
// If any action taken in rows/cols/diags pass 1 so that subsequent actions won't execute.
// Try to win, or defend based on Action variable passed from below.

    int diags=0;
    int rowscols=0;

    diags = Scan_diag(xo,Fig_PC,Fig_Player,Action);
    rowscols = Scan_rowscols(xo,Fig_PC,Fig_Player,Action);

    if (rowscols || diags)
    {
        return 1;
    }

    return 0;

}


int Scan_diag(char (*xo)[size],char Attk_fig,char Def_fig,int Action)
{
    int i=0;
    int index=0;
    int count_diag=0;

    for (i=0;i<size;++i)
    {
        if (xo[i][i]==Attk_fig)
        {
            ++count_diag;
        }
        else if (xo[i][i]==' ')
        {
            count_diag+=100;
            index=i;
        }
    }

    if (count_diag==102)
    {
        if (Action)
        {

        xo[index][index]=Attk_fig;
        return 1;
        }

        xo[index][index]=Def_fig;
        return 1;
    }

    return 0;
}



int Scan_rowscols(char (*xo)[size],char Attk_fig,char Def_fig,int Action)
{
    int i=0;
    int j=0;
    int index1=0;
    int index2=0;
    int count_row=0;
    int count_col=0;

    for (i=0;i<size;++i)
    {
        for (j=0;j<size;++j)
        {
            // count in rows
             if (xo[i][j]==Attk_fig)
             {
                 ++count_row;
             }
             else if (xo[i][j]==' ')
             {
                 count_row+=100;
                 index1=j;
             }
             // count in cols
             if (xo[j][i]==Attk_fig)
             {
                 ++count_col;
             }
             else if (xo[j][i]==' ')
             {
                 count_col+=100;
                 index2=j;
             }
        }


        if (count_row==102)
        {
            if (Action)
            {
                xo[i][index1]=Attk_fig;
                return 1;
            }
            else
            {
                xo[i][index1]=Def_fig;
                return 1;
            }
        }
        else
        {
            count_row=0;
        }


        if (count_col==102)
        {
            if (Action)
            {
                xo[index2][i]=Attk_fig;
                return 1;
            }
            else
            {
                xo[index2][i]=Def_fig;
                return 1;
            }
        }
        else
        {
            count_col=0;
        }

    }

    return 0;
}



void Scan_corners(char (*xo)[size],char Fig_PC,char Fig_Player)
{
    int i=0;
    int j=0;
    int ii=0;
    int jj=0;
    int count=0;

    for (i=0;i<size;i+=(size-1))
    {
        for (j=0;j<size;j+=(size-1))
        {
            if (xo[i][j]==' ')
            {
                ++count;
                ii=i;
                jj=j;
            }
        }
    }
    switch (count)
    {
    case 1:
        xo[ii][jj]=Fig_PC;
        break;


    case 2:
        if ((xo[0][size-1]==Fig_PC) || (xo[0][size-1]==Fig_Player))
        {
            xo[0][0]=Fig_PC;
        }
        else
        {
            xo[0][size-1]=Fig_PC;
        }
        break;


    case 3:
        if (xo[((size-1)/2)][((size-1)/2)]==Fig_Player)
        {
            xo[0][size-1]=Fig_PC;
        }
        else if (xo[size-2][0]==Fig_Player)
        {
            xo[size-1][size-1]=Fig_PC;
        }
        else
        {
            xo[0][0]=Fig_PC;
        }
        break;


    case 4:
        xo[size-1][0]=Fig_PC;
        break;
    }
}


int Check_board(char (*xo)[size],char Fig_PC, int *PC_move)
{
    int i=0;
    int j=0;
    int count_row=0;
    int count_col=0;
    int count_diag1=0;
    int count_diag2=0;

    if (*PC_move==5)
    {
        printf("It's a draw\n\n");
        return 0;
    }


    for (i=0;i<size;++i)
    {
        if (xo[i][i]==Fig_PC)
        {
            ++count_diag1;
        }

        if (xo[i][(size-1)-i]==Fig_PC)
        {
            ++count_diag2;
        }

        for (j=0;j<size;++j)
        {
            if (xo[i][j]==Fig_PC)
            {
                ++count_row;
            }

            if (xo[j][i]==Fig_PC)
            {
                ++count_col;
            }
        }
        if ((count_row==size) || (count_col==size))
        {
            printf("PC wins\n\n");
            return 0;
        }
        else
        {
            count_row=0;
            count_col=0;
        }
    }

    if ((count_diag1==size) || (count_diag2==size))
    {
        printf("PC wins\n\n");
        return 0;
    }


return 1;

}
\$\endgroup\$
26
\$\begingroup\$

Here are some things that may help you improve your code.

Fix your formatting

The indentation, in particular, seems rather random. It may be that it's an artifact of posting the code, but it doesn't look very consistent. It's less important which coding convention you follow, than that you consistently follow some convention.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments: "I didn't know that." or "That's bad advice!" My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard. For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

[...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree, (often vehemently!) In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.

Eliminate unused variables

Unused variables are a sign of poor code quality, so eliminating them should be a priority. In this code, i within Print_board is defined and assigned a value but then never used. Your compiler is probably also smart enough to tell you that, if you ask it to do so.

Make sure you have all required #includes

The code uses usleep but doesn't #include <unistd.h>. But with that said, see the next suggestion.

Avoid obsolete functions

The usleep call is a POSIX rather than a C standard function, but has been obsolete since POSIX.1-2001. You could either use nanosleep which is the new POSIX function or, since you are trying to delay 1 second and already have <time.h> included, you could use sleep(1); instead.

Use whitespace to improve readability

Lines like this:

for (i=0;i<size;++i)

become much easier to read with a little bit of whitespace:

for (i = 0; i < size; ++i)

Use named constants

Instead of having "magic numbers" like 100 and 102 within the code, it would make more sense to make them named variables. This increases the readability and maintainability of the code and costs nothing in terms of performance.

Be consistent with initialization of variables

The xo variable is currently declared and initialized like this:

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

but that's not quite right. It's declared as 3 sets of 3, so the initialization should instead be:

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

Use const where practical

The Fig_PC and Fig_Player variables don't ever and shouldn't ever change and therefore should be declared const.

Declare variables in the smallest practical scope

By declaring variables in the smallest practical scope, you reduce the chance for name collisions and make it clear to the reader of your code where variables are and are not needed. With any C compiler conforming to the 1999 specification (which should be all of them at this point!) one could rewrite Clear_board like this for example:

void Clear_board(char (*xo)[size])
{
    for (int i = 0; i < size; ++i) {
        for (int j = 0; j < size; ++j) {
            xo[i][j] = ' ';
        }
    }
}

The variables i and j are declared within the scope of their respective for loops. Although as a practical matter, I'd write it differently; see the next suggestion.

Use library functions where appropriate

The Clear_board function sets all of the members of xo to the ' ' character, but there's already a library function that does that very efficiently. Use it like this:

void Clear_board(char (*xo)[size])
{
    memset(xo, ' ', size*size);
}

Think of the user

The square numbering is only shown once at the beginning of the game, so if the player doesn't remember that the squares are numbered from bottom to top (like the numbers on the number portion of a keyboard) and instead thinks they might be numbered from top to bottom (like the numbers on a telephone keypad), that player is likely to be frustrated. Also, if the player enters an already occupied space, the game interprets that as simply forfeiting the turn which doesn't seem quite right.

Check return values of standard functions

Many standard functions, including scanf can fail and return values that can help in detecting those failures. To write robust software, you should get into the habit of checking the return values and dealing intelligently with the potential failures.

Use stdbool

Instead of returning 1 and 0 from functions like Action and Scan_diag, you might consider instead using #include <stdbool.h> which defines true and false and would allow you to declare that those functions return type bool which is more descriptive of their actual function.

Combine declaration and initialization where practical

The code currently has a number of places where a variable is initialized twice. An example from Action() is this:

int diags = 0;
int rowscols = 0;
diags = Scan_diag(xo, Fig_PC, Fig_Player, Action);
rowscols = Scan_rowscols(xo, Fig_PC, Fig_Player, Action);

It would be more logical and more compact to write it like this instead:

int diags = Scan_diag(xo, Fig_PC, Fig_Player, Action);
int rowscols = Scan_rowscols(xo, Fig_PC, Fig_Player, Action);

Store the game state in a struct

Right now there are two essential things that represent the game state, which are the board itself and the two constants representing each player. Additionally, the number of turns is tracked and incremented. It would make the parameter passing a little less messy if all of these were collected into a single struct. Then the functions could mostly take a single pointer to that struct instance.

\$\endgroup\$
  • \$\begingroup\$ @vaxquis In a code review, I offer suggestions, not rules. It's been discussed here, too \$\endgroup\$ – Edward Jun 11 '16 at 14:06
  • \$\begingroup\$ You misread. It was a sloppy existing practice because it used to be undefined behavior before the standard was changed. Now, however, it's explicitly defined behavior. It wasn't that long ago that defining a variable inside a for loop was an error. Now it's a best practice. Programming languages, like spoken languages, change over time. \$\endgroup\$ – Edward Jun 11 '16 at 21:37
  • \$\begingroup\$ Here's another guy with an opinion. Unlike either of us, however, he created the C++ language. \$\endgroup\$ – Edward Jun 11 '16 at 21:56
  • \$\begingroup\$ Let us continue this discussion in chat. \$\endgroup\$ – vaxquis Jun 11 '16 at 21:57
11
\$\begingroup\$

Inconsistent Indentation

The code in Main is not indented.

The code in PC_turn is not properly indented. The code in Print_board is not properly indented.

Example of proper indentation:

void PC_turn(char (*xo)[size],char Fig_PC,char Fig_Player,int *PC_move)
{
    printf("Computer's Turn:\n");

    /*
     First try to win by completing an X X X
     Second priority is to defend
     Lastly try to take postion of corners
     */

    if (!Action(xo,Fig_PC,Fig_Player,1))
    {
        if (!Action(xo,Fig_Player,Fig_PC,0))
        {
            Scan_corners(xo,Fig_PC,Fig_Player);
        }
    }

    Print_board(xo);
    ++*PC_move;
}

You have a bug in your code, I got a draw once when the computer actually won.

 X | X | O
___|___|___ 
   |   |    
 X | O | O
___|___|___ 
   |   |    
 X | O | X
   |   |    


It's a draw
\$\endgroup\$
  • 5
    \$\begingroup\$ I can verify the bug. It happens when the player enters 5, 2, 9, 6. Good catch! \$\endgroup\$ – Edward Jun 10 '16 at 16:58

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.