Basic Tic-Tac-Toe

I am starting a software engineering degree soon and have been practicing the last couple months. I have posted some code I wrote for a basic Tic Tac Toe game below (C++). I have read the worst thing to do is to get into bad habits early on, so please can you read over the code and tell me what I should be doing differently? Be as brutally honest as possible, I care more about my future than feelings. Bad practices, bad code, bad formatting, anything.

```
#include
#include

void playGame();
void printBoard(char board[3][3]);
int nextMove(char symbol, int plays, int playedMoves[9]);
bool checkWinner(char board[3][3], char symbol, int plays);

int main()
{
using namespace std;

cout > yn;

//following 2 lines are to handle incorrect inputs
cin.clear();
cin.ignore(numeric_limits::max(), '\n');
}

if (tolower(yn) == 'y')
playGame();
else
break;
}

return 0;
}

//Where the game starts. Sets up board and runs 9 plays
void playGame()
{
using namespace std;

char board[3][3] =
{
{ ' ', ' ', ' ', },
{ ' ', ' ', ' ', },
{ ' ', ' ', ' ' }
};
printBoard(board);

char symbol = ' ';
int move = 0;
int playedMoves[9] = { 0 };

for (int plays = 1; plays 4)
if (checkWinner(board, symbol, plays)) break;
}
}

//Prints the board
void printBoard (char board[3][3])
{
using namespace std;

string sLine = "";

cout 9)
{
cout > move;

if (!cin)
{
cin.clear();
cin.ignore(numeric_limits::max(), '\n');
}
}

validPlay = true;

for (int k = 0; k < plays; k++)
{
if (move == playedMoves[k]) validPlay = false;
}
}

return move;
}

//Check to see if a player has won or if there is a tie
bool checkWinner(char board[3][3], char symbol, int plays)
{
using namespace

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

Use the appropriate #includes

This code uses numeric_limits which is actually defined in `. Even if your code compiles that way, it's probably only because one of the other files happened to include it. You can't and shouldn't rely on that, though.

Use objects

You have a board structure and then separate functions printBoard and checkWinner that operate on board data. With only a slight syntax change, you would have a real object instead of C-style code written in C++. You could declare a TicTacToe object and then play, print and checkWinner could all be member functions.

Separate I/O from program logic

Right now every individual function has both game logic and I/O. It's often better design to separate the two so that the game logic is independent of the I/O with the user.

Use const where practical

I would not expect the checkWinner or printBoard routines to alter the underlying board on which they operate, and indeed they do not. You should make this expectation explicit by using the const keyword:

void printBoard(const char board[3][3]);

This declares that the printBoard will not modify the passed board, making it clear to both the compiler and to the human reader of your code.

Create a function rather than repeating code

In several places in the code, a prompt is printed, then a answer read, then the input cleared. Instead of repeating it, make it into a function.

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. Having it inside every function is only slightly better. For instance, it's entirely superfluous in the playGame function and should be omitted. For this program, I'd advocate removing it everywhere and using the std:: prefix where needed.

Think of the user

It's not intuitively obvious how to enter a move when the game first starts. It would be better to consider the user and to offer a prompt (at least at the beginning) showing how the squares are numbered.

Fix the bug

If a player enters a square that is already filled, the program simply hangs. That's not good behavior and must be considered a bug.

Consider altering your algorithm

Right now, the checkWinner routine is only called if more than four moves have been made. Since this code is not particularly performance sensitive, why not simply call it every time? It would make the code a little simpler to read and the extra time taken will almost certainly never be noticed.

Declare the loop exit condition at the top

The for loop inside playGame currently says this:

for (int plays = 1; plays < 10; plays++)

Reading that line, we would conclude that the play continues until plays >= 10. However, way down at the bottom of the loop is a break that occurs if one player has won. Rather than forcing the reader of the code to examine every line, it's better if you simply declare loop exit conditions completely and honestly at the top.

Put conditional statements on a separate line

The code currently includes a number of places where something like this is done:

if (l < 2) sLine += "|";

It makes things a little bit harder to read than if you put them on separate lines like this:

if (l < 2) {
    sLine += "|";
}

Further, especially when you are beginning, it's useful to always put the curly braces there. Doing so will make your intentions clear to both readers of the code and the compiler and can reduce the possibility for certain kinds of subtle bugs like this:

for (int i = 0; i < 3; ++i)
    f[k] = k*2;
    g[k] = f*7;

By the indentation, one would expect that both statements are executed every loop iteration, but the subtle lack of braces means that the compiler will do something else entirely.

Use meaningful variable names

Your function names are descriptive and good enough, but the variable names are not so good. In particular the printBoard routine uses a loop counter named l which is a particularly bad choice of variable name. It's too easily mistaken for the digit 1 or the letter i.

Eliminate "magic numbers"

This code has a number of "magic numbers," that is, unnamed constants such as 2, 9, 10, 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.

Eliminate return 0 at the end of main

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

StackExchange Code Review Q#93429, answer score: 16