Basic Tic-Tac-Toe

Question

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 <iostream>
#include <string>

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 << "Tic Tac Toe by Kevin\n"
        << "____________________\n\n";

    playGame();

    char yn = 'a';

    //Check if players want another game
    while (true)
    {
        while (tolower(yn) != 'y' && tolower(yn) != 'n')
        {
            cout << "Do you want to play again? (y/n): ";
            cin >> yn;

            //following 2 lines are to handle incorrect inputs
            cin.clear();
            cin.ignore(numeric_limits<streamsize>::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 < 10; plays++)
    {
        if (!(plays % 2))
            symbol = 'O';
        else
            symbol = 'X';

        move = nextMove(symbol, plays, playedMoves);
        playedMoves[plays - 1] = move;

        int x = 0, y = 0;
        x = (move + 2) % 3;
        y = (move - 1) / 3; //math to convert 1-9 to [][]
        board[x][y] = symbol;

        printBoard(board);

        if (plays > 4)
            if (checkWinner(board, symbol, plays)) break;
    }
}

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

    string sLine = "";

    cout << endl;

    for (int k = 0; k < 3; k++)
    {
        sLine = "";

        for (int l = 0; l < 3; l++)
        {
            sLine += board[l][k];
            if (l < 2) sLine += "|";
        }

        cout << sLine << endl;
        if (k < 2) cout << "-----" << endl;
    }

    cout << endl;
}

//Takes next move and checks for errors
int nextMove (char symbol, int plays, int playedMoves[9])
{
    using namespace std;

    bool validPlay = false;
    int move = 0;

    while (!validPlay)
    {
        while (move < 1 || move > 9)
        {
            cout << "Player " << symbol << " enter your move: ";
            cin >> move;

            if (!cin)
            {
                cin.clear();
                cin.ignore(numeric_limits<streamsize>::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 std;

    bool winner = false;

    //Check vertical and horizontal
    for (int k = 0; k < 3; k++)
    {
        if (board[k][0] == symbol && board[k][1] == symbol && board[k][2] == symbol)
            winner = true;
        if (board[0][k] == symbol && board[1][k] == symbol && board[2][k] == symbol)
            winner = true;
    }

    //Check diagonal
    if (board[0][0] == symbol && board[1][1] == symbol && board[2][2] == symbol)
        winner = true;
    if (board[2][0] == symbol && board[1][1] == symbol && board[0][2] == symbol)
        winner = true;

    if (winner)
        cout << "Congratulations! The winner is " << symbol << "'s!\n\n";
    if (!(winner) && (plays == 9))
        cout << "Unfortunately it was a tie!\n\n";

    return winner;
}

Answer 1

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 <limits>. 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.

Answer 2

This is very well-written "beginner" code.

To add to @Edward's already great answer, here are a few style considerations (i.e. opinions):

Prefer prefix increment

for (int plays = 1; plays < 10; plays++) // bad style
for (int plays = 1; plays < 10; ++plays) // good style

You'll often see people complain when you use i++ instead of ++i in for loops.

This is because these are two different operators with different semantics.

In the case of lightweight integers and pointers, there is no difference, but there can actually be a performance penalty or even bugs due to using i++. This is particularly true for pure input iterators, such as istream_iterator.

Avoid declaring multiple variables on the same line

int x = 0, y = 0;

Many people consider this bad style, because it gets very confusing for more complex types.

For example, with pointers:

int* x = NULL, y = NULL;

This code is incorrect, because x is a pointer, but y is a normal int. The corrected version is:

int *x = NULL, *y = NULL;

This is caused by C++'s weird inside-out type syntax.

The preferred style is usually:

int x = 0;
int y = 0;

Combine related if blocks

Inside your checkWinner function, there are a couple of instances of:

if (something)
    winner = true;
if (something_else)
    winner = true;

That can (and probably should) be transformed into:

if (something || something_else) {
    winner = true;
}

Also, at the end of your checkWinner function, we find this:

if (winner)
    cout << "Congratulations! The winner is " << symbol << "'s!\n\n";
if (!(winner) && (plays == 9))
    cout << "Unfortunately it was a tie!\n\n";

That can be transformed into this:

if (winner) {
    cout << "Congratulations! The winner is " << symbol << "'s!\n\n";
} else if (plays == 9) {
    cout << "Unfortunately it was a tie!\n\n";
}

Note: Please, please, please, put braces around your if blocks.

Regarding using namespace std;

Many people have a great distaste for using namespace. It causes "namespace pollution", which can cause name conflicts later on.

Generally, using namespace is meant for sort of "inheriting" a namespace. Your namespace is now a partial owner to everything in the namespace you pulled in.

Consider:

#include <string>

struct string
{
    char* data;
    int length;
};

int main()
{
    using namespace std;
    string str;
    // error: reference to 'string' is ambiguous
}

In general, a much better solution is to either prefix things with std:: or only pull in the things you need.

For example, your main:

int main()
{
    using std::cin;
    using std::cout;
    using std::tolower;
    using std::numeric_limits;
    using std::streamsize;

The advantage of this style is that you know exactly what each function depends on. However, it can get very messy for functions that depend on a lot of things.

Another method that can be used for long or nested namespace names is namespace aliases:

#include <functional> // namespace std { namespace placeholders { struct {} _1; } }

void func(int);

int main()
{
    namespace ph = std::placeholders;
    std::bind(func, ph::_1)(7);
    // No need to understand this, the key is using `ph::_1` instead of `std::placeholders::_1`.
}

Answer 3

I would change this:

playGame();

char yn = 'a';

//Check if players want another game
while (true)
{
    while (tolower(yn) != 'y' && tolower(yn) != 'n')
    {
        cout << "Do you want to play again? (y/n): ";
        cin >> yn;

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

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

to

char yn = 'y';
while (tolower(yn) == 'y')
{
    playGame();

    //Check if players want another game
    cout << "Do you want to play again? (y/n): ";
    cin >> yn;
    while (tolower(yn) != 'y' && tolower(yn) != 'n')
    {
        cout << "Do you want to play again? (y/n): ";
        cin >> yn;

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

Answer 4

Bug

You also have a bug with asking if the user want to play again as yn is never reset once set to 'y' and the game will always start over.

Intuitiveness

Maybe declare your board as follows to be more intuitive.

Also consider swapping x and y as the view in code is rotated

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