Tic Tac Toe console program

Question

I have been learning C++ for 3 months now. I decided to make something by myself, so I've made this little integer-based TIC TAC TOE console game.

I need some advice on how to improve my code for better structure and and better readability.

#include <iostream>
#include <stdlib.h>


using namespace std;

int arr[9]={0,1,2,3,4,5,6,7,8}; //Global values of the GAME BOARD for Every move solt 

void gameboard(int x);      // function for taking the turn of the player and OVERWRITE the player's value i.e [69 for player 1 or 96 for player 2] to the GLOBAL gameboard array as a slot ie [ arr[turn value of the player] ]
void turn(int *player);     // function for checking and validating  player's move and pass that to to the game board function 
void check_win(int player); // function for checking if a player has own or the game has drawn
void initial_board();       //function for printig the initial GAME BOARD before any players turn
void game_over();           //function for endgame condition 

int main(int argc, char** argv) {
    int a=10,b=11;       // player 1 == 69 and player 2 == 96

    initial_board();    // printing the initail GAME BOARD for all the empty slots availabe for turn

    while(true){        //initiating an infinite loop untill a player has own the game or the game has drawn
        turn(&a);       // executing the function for PLAYER 1's TURN [69]
        turn(&b);       // executing the function for PLAYER 2's TURN [96]  
    }

    return 0;
}

void gameboard(int x,int player){ // pass cohice as x and p as player
    system("cls");
    cout<<"        TIC TAC TOE\t\n\n";
    cout<<" Player 1: 10 -- Player 2: 11\n\n";
    arr[x]=player;       // overwrite the element of the array that is the turn input or choice to the player value[example: if (choice is 1 and player valuse is 69) then arr[1]=69 ]
    for(int i=0;i<9;i++){   // display the game board
        if(i%3==0&&i!=0){   // formatting condition
            cout<<"\n";
            cout<<"\n";
        }    
        cout<<"   | "<<arr[i]<<" | ";    // formatting   
    }
    cout<<"\n";
    check_win(player);  // finally check for win and draw conditions and exit if any of them satisfies 
}

void turn(int *player){     // [*player] pass by ref for either player 1 [69] or player 2 [96],,though it can be using pass by valuse as well 
    int choice,p;           // input choice variable for turn and p variable for player value and passing that to the game_board function
    p= *player;             // pass the value of player ie 69 or 96 to p variable 
    cout<<"\n"<<" "<<p<<" it is your Turn!!";
    cin>>choice;            // input turn as choice
    if(choice > 8){         // validate if choice is over 8 or not 
        do{
            cout<<"Out of range: Try again-->";
            cin>>choice;
        }while(choice > 8);  // end validation and go for next validation
    }   
    for(int i=0; i<9; i++)  // iterate the global slot array 
    {
        if(arr[choice]==10||arr[choice]==11)  // check if the turn or choice already exist as player values ie[69 or 96] in the global array
        {
            do{                               // if so then,retake the move because taht is already preasant in the board
                cout<<"invalid move!!";
                cout<<"place your turn!!";
                cin>>choice;                 // input turn as choice(if choice is under 8) again until it does not exists in the global GAMEVOARD array as player value
                if(choice > 8){             //another check for choice over 8. if so,then repeaat input untill its under 8 and doesnt exists in the global gameboard array as player value
                    do{
                        cout<<"Out of range: Try again-->";
                        cin>>choice;
                    }while(choice > 8);
                }   
            }while(arr[choice]==10||arr[choice]==11);
            break;                        // if all the validation condition satisfies then breakout of the loop nad the input[choice] as final value 
        }   
    }
    gameboard(choice,p);                 // pass the final turn input ie choice to the gameboard function and pass the player value as p            
}

/// Need to modify win conditions later ////

void check_win(int player){              //pass the player value as player
    if(arr[1]+arr[2]+arr[0]==3*10||arr[1]+arr[2]+arr[0]==3*11){     //win condition  outputs the winner player's name for all
        cout<<"\n\n\n"<<" player "<<player<<" has won the game.Congratulations!!!"<<endl;  
        game_over();
    }else if(arr[3]+arr[4]+arr[5]==3*10||arr[3]+arr[4]+arr[5]==3*11){  //win condition  outputs the winner player's name for all
        cout<<"\n\n\n"<<" player "<<player<<" has won the game.Congratulations!!!"<<endl;
        game_over();
    }else if(arr[6]+arr[7]+arr[8]==3*10||arr[6]+arr[7]+arr[8]==3*11){  //win condition  outputs the winner player's name for all
        cout<<"\n\n\n"<<" player "<<player<<" has won the game.Congratulations!!!"<<endl;
        game_over();
    }else if(arr[0]+arr[4]+arr[8]==3*10||arr[0]+arr[4]+arr[8]==3*11){  //win condition  outputs the winner player's name for all
        cout<<"\n\n\n"<<" player "<<player<<" has won the game.Congratulations!!!"<<endl;
        game_over();
    }else if(arr[2]+arr[4]+arr[6]==3*10||arr[2]+arr[4]+arr[6]==3*11){  //win condition  outputs the winner player's name for all
        cout<<"\n\n\n"<<" player "<<player<<" has won the game.Congratulations!!!"<<endl;
        game_over();
    }else if(arr[0]+arr[3]+arr[6]==3*10||arr[0]+arr[3]+arr[6]==3*11){  //win condition  outputs the winner player's name for all
        cout<<"\n\n\n"<<" player "<<player<<" has won the game.Congratulations!!!"<<endl;
        game_over();
    }else if(arr[1]+arr[4]+arr[7]==3*10||arr[1]+arr[7]+arr[7]==3*11){  //win condition  outputs the winner player's name for all
        cout<<"\n\n\n"<<" player "<<player<<" has won the game.Congratulations!!!"<<endl;
        game_over();
    }else if(arr[2]+arr[5]+arr[8]==3*10||arr[2]+arr[5]+arr[8]==3*11){  //win condition  outputs the winner player's name for all
        cout<<"\n\n\n"<<" player "<<player<<" has won the game.Congratulations!!!"<<endl;
        game_over();
    }else if(arr[0] != 0 && arr[1] != 1 && arr[2] != 2 && arr[3] != 3 && arr[4] != 4 && arr[5] != 5 && arr[6] != 6 && arr[7] != 7 && arr[8] != 8){
        cout<<"\n\n\n THE GEME IS DRAWN!!!!!!";                       // draw condition
        game_over();
    }                   
}

void initial_board(){   
    cout<<"        TIC TAC TOE\t\n\n";
    cout<<" Player 1: 10 -- Player 2: 11\n\n";

    for(int i=0;i<9;i++){   // iterating the global array to show all the slots
        if(i%3==0&&i!=0){   // formatting the slots
            cout<<"\n";
            cout<<"\n";
        }   
        cout<<"   | "<<arr[i]<<" | ";    // printing the slot values as slots
    }
    cout<<"\n";   //formatting
}

void game_over(){                        // function for ending the game 
    cout<<" \n\n :The Game is over:\n\n";
    system("pause");
    exit(1);
}
/*
    #########################################
    #########     END OF PROJECT    #########
    #########################################
*/
```

Answer 1

This is not bad for a programmer as new to C++ as you have said you are. Keep up the good work! With that said, here are some ideas on how you might be able to improve your program.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. It's not necessarily wrong to use it, but you should be aware of when not to (as when writing code that will be in a header).

Don't use system("cls")

There are two reasons not to use system("cls") or system("pause"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into separate functions cls() and pause(), and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++.

Use the appropriate #includes

This code has #include <stdlib.h> but in a C++ program that should actually be <cstdlib> which puts the various declarations into the std:: namespace rather than in the global namespace. However in this case, if you follow the immediately previous suggestion, the code will use nothing from that and so the #include can simply be eliminated.

Use whitespace to improve readability

Lines like this:

}while(arr[choice]==10||arr[choice]==11);

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

} while (arr[choice] == 10 || arr[choice] == 11);

Use constant string concatenation

This code currently has a number of instances that look like this:

cout<<"invalid move!!";
cout<<"place your turn!!";

This calls the << operator twice. Instead, you could write this:

std::cout << "invalid move!!\n"
             "place your turn!!";

This only calls << once. The compiler automatically concatenates the string literals together.

Consider using standard length lines

The comments in the code are very long and some lines are over 200 characters. Neither of these things are necessarily wrong, but they are different from the usual convention which is to make sure that lines are no more than around 80 characters long. Some use 132 characters as a limit. Both of these stem from historical limitations of standard printers and displays, and while those are history, the convention is still widely used.

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. The global variable arr (which is not a good name, by the way) could instead be wrapped in an object to make it easy to differentiate between read access and an update and to keep the state of the game consistent and accurate.

Don't Repeat Yourself (DRY)

The checkwin() function includes large portions of repeated code. Instead of repeating code, it's generally better to make common code into a function.

Omit unused variables

Because argc and argv are unused, you could use the alternative form of main:

int main ()

Eliminate "magic numbers"

This code has a number of inscrutable "magic numbers," that is, unnamed constants such as 3, 10, 11, etc. Generally it's better to avoid that and give such constants meaningful names. That way, if anything ever needs to be changed, you won't have to go hunting through the code for all instances of "3" and then trying to determine if this particular 3 is relevant to the desired change or if it is some other constant that happens to have the same value.

Use longer, more meaningful names

Names like p and a and b and arr are not very descriptive. Better names help the reader of the code understand what is happening and why.

Don't mislead with comments

The code currently contains this peculiar line:

int a = 10, b = 11;         // player 1 == 69 and player 2 == 96

The comments and the code contradict each other. If the variables were given better names, you could simply eliminate the comment:

int player1 = 10;
int player2 = 11;

Declare each variable on its own line

Declaring each variable on its own line avoids certain kinds of errors and leaves more room for a meaningful comment, if needed. See ES.10

Use rational return values

All of your functions return void and half take no parameters. This is highly suspect. For example, it would make more sense to have turn return a bool value to indicate that the player who just played has won the game. See the next suggestion.

Declare the loop exit condition at the top

The main routine has this seemingly endless loop:

while (true) {
    turn(&a);
    turn(&b);
}

However the comment indicates that it's not really an infinite loop, but that the game ends when either one player wins or the game is a tie. It would be better to make that explicit in the code itself rather than just in the comment.

while ((!turn(&a) && !tie()) && (!turn(&b) && !tie()))
{ /* keep playing until game end */ }

This uses short-circuit evaluation, which means that, in the case of an && expression, if the first condition is not true, the second one won't be evaluated. In this case, we assume that turn returns true if that player has just won and that a new function tie returns true if the game is a tie.

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 the game (which does not depend on the underlying OS).

Have you run a spell check on comments?

If you run a spell check on your comments, you'll find a number of things such as "GAMEVOARD" instead of "GAMEBOARD" and "nad" instead of "and". Also in the case of a tie, the user is informed that "THE GEME IS OVER" (instead of the "GAME"). It's worth the extra step to eliminate spelling errors, especially in user-visible strings.

Think of the user

The game seems to work and follow the usual conventions for the game, but the use of 10 and 11 is a bit strange for users. Consider using instead the typical text characters X and O. There is some indication that perhaps you tried to do that. A hint in that regard: if you keep letters in the game array instead of int values, you may find it easier to manage.

Don't pass raw pointers

The turn() function is passed a pointer to the player value which is not a good idea. Modern C++ makes very little use of raw pointers. Instead, it would make more sense to do it as was done for the check_win() code and simply pass by value since what's being passed is not a gigantic structure (that would take time and memory to copy) but just an int which likely fits in a machine register.

Eliminate return 0

You don't need to explicitly provide a return 0; at the end of main -- it's created implicitly by the compiler.

Answer 2

There's a typo in this line:

}else if(arr[1]+arr[4]+arr[7]==3*10||arr[1]+arr[7]+arr[7]==3*11){  //win condition  outputs the winner player's name for all

I think that the second 7 was intended to be 4.

That's the kind of thing that tends to happen when you repeat large blocks of similar code. And most readers won't spot that.

What you should do is identify the patterns in your groups:

• Horizontal lines start at 0, 3 or 6, with step size of 1
• Vertical lines start at 0, 1 or 2, with step size of 3
• Leading diagonal starts at 0, with step size of 4
• Trailing diagonal starts at 2, with step size of 2

With that observation, it should be easy to replace those lines with simple function calls. That will make your code easier to write and to review.

Answer 3

Your conditions testing for victory have a bug, due to your choice of data structure.

In the array arr, a number from 0 to 8 represents an empty space, and a number 10 or 11 represents "X" or "O". (This is what I infer from your code; it would have been helpful to document it using a comment where the array is declared.)

It follows that the condition arr[6] + arr[7] + arr[8] == 3*10 is true when the first player has played in cells 6, 7 and 8. However, because 11 + 11 + 8 = 30, it is also true when the second player has played in cells 6 and 7, and cell 8 remains empty. The same applies for other win conditions involving cell 8.

This bug could be fixed by using different numbers to represent the players' moves. I would suggest using 1, -1 and 0 to represent "X", "O" and "empty" respectively. There is no need for different empty cells to be represented in the data structure by different numbers; if you want to print the board with numbers in the empty cells, then the code to achieve that belongs in the function which prints the board.

Answer 4

You asked about structure and readability, so this answer will mostly be about that, and I will not include much code, only concepts.

First off, your console app is really more "C" than "C++" - it is procedural, rather than object-oriented, so you're not really using the ++ here. There's nothing really wrong with that, unless you want to learn object-oriented programming. Generally when someone says "I'm learning C++" we will want to see them doing object-oriented stuff, otherwise they would say they are learning C.

Tic-tac-toe lends itself very nicely to beginning object-oriented concepts, and there are many design patterns which could be used to implement a Tic-tac-toe game! It is a very good choice of game for learning these ideas.

Example design pattern - MVC (model-view-controller) - https://en.wikipedia.org/wiki/Model%E2%80%93view%E2%80%93controller

Most computer programs need to create a logical "model" of some "thing" in the real world, in this case it is a game board for Tic-tac-toe. In your app, you are using an array of numbers for this. In a C++ app, we would use a class that represents a game board, and is responsible for things like storing the current board state, determining if a player's attempted move is legal, determining if the game has been won, the size of the board(!) and other things like that. These "things" can be divided into "data" and "actions" - which will become your member variables and methods, respectively. This class would become your MODEL in the MVC pattern.

In order to display things on the screen, you would need a class that handles that sort of thing, which would become your VIEW in the MVC pattern. This class would handle the specifics of whatever output device you are using, and you could even have multiple views that display the same MODEL, so you could draw different graphics on a phone, than you would on a web browser, or you could even create a physical game board if you were using Arduino for example. All of those different views could use the exact same MODEL class described above. The view could also create various user interface elements that work for the output it is creating - for example, the phone view could respond to screen touches, while the Arduino view would respond to button presses.

In order to "glue" your model and view together, that is where the CONTROLLER comes in. The controller is responsible for taking data from the model and passing it to the view, or taking data from the view/user and passing it to the model. In this way, your view doesn't need to contain any game logic, and your model doesn't need any display or user interface logic, making BOTH objects way more useful.

There is already an example of this on the site: Tic Tac Toe in MVC

It is in Java, but it is very similar to C++, so it should be readable to you.

The MVC pattern is a natural result of applying the "Single Responsibility Principle" which can be applied at many levels from the application down to the individual blocks of code. Every class should have a single responsibility (such as "manage the game board data"), but so also should every function within each class. So, you wouldn't have a class that handled the model and the view, and you wouldn't have a function that, for example, enters the players move AND checks if they won. You would have two separate functions. This makes your code both more readable and more useful. (For example, because in the first 2 moves, the controller doesn't need to check for a winner, and for larger boards, this number becomes even larger, providing a possible performance improvement at the beginning of the game.)

https://en.wikipedia.org/wiki/Single_responsibility_principle

I hope that is helpful. I know I'm suggesting completely re-working the whole thing, but if you want to create well structured and readable C++, we need to be doing objects.