Tic Tac Toe in C++11

Question

I wrote this to sort of gauge my understanding of the bare basics of C++.Could you guys give me any pointers? (Ha, get it?). I would happy to hear what you have to say about the code. Also is it neat (aesthetically as well as functionally)?

#include <iostream>
bool end = false;
int w_combinations[8][3] = {{0,1,2},{3,4,5},{6,7,8},{0,3,6},{1,4,7},{2,5,8},{0,4,8},{2,4,6}};
int pos[] = {1,2,3,4,5,6,7,8,9};
void check (int x) { 
    if ( x == 10) {
        std::cout << "X|";
    }
    else if (x == 11) {
        std::cout << "O|";
    }
    else {
        std::cout << x << "|";
    }
}
void printMatrix() {
    std::cout << "|" ; check(pos[0]) ; check(pos[1]) ; check(pos[2]);std::cout << "\n";
    std::cout << "|" ; check(pos[3]);check(pos[4]) ; check(pos[5]);std::cout << "\n";
    std::cout << "|" ; check(pos[6]) ; check(pos[7])  ; check(pos[8]); std::cout << "\n";
}
void input(int z , int player) {
    if (player == 1 && pos[z-1] != 10 && pos[z-1] != 11) {
        pos[z-1] = 10;
    } 
    else if (player == 2 && pos[z-1] != 10 && pos[z-1] != 11){
        pos[z-1] = 11;
    }
    else {
        std::cout << "\nInvalid! Try again: ";
        int y;
        std::cin >> y;
        input(y,player);

    }
}

//Winning mechanism: Simply cylce through all combos.
bool win() {
    for (int i = 0 ; i < 8 ; ++i) {
        if (pos[w_combinations[i][0]] == pos[w_combinations[i][1]] && pos[w_combinations[i][0]] == pos[w_combinations[i][2]]) 
        {
            return true;
            break;
        }
    }
    return false;
}
int main() {
    int count = 0;          //Cycle through draws
    while (1) 
    {
        int z;
        printMatrix();
        std::cout << "1[X]:";
        std::cin >> z;
        input(z,1);
        if (win()) {std::cout << "Player 1 wins!\n";break;}
        ++count;
        printMatrix();
        if (count == 9) {
            std::cout << "Draw!";
            break;
        }
        std::cout << "2[O]:";
        std::cin >> z;
        input(z,2);
        if (win()) {std::cout << "Player 2 wins!\n";break;}
        ++count;
        if (count == 9) {
            std::cout << "Draw!";
            break;
        }
    }
    std::cout << "Play again[Yes = 1, No = 0]: ";
    int permission;
    std::cin >> permission;
    if (permission == 1) {
        std::cout << "\n\n";
        main();
    }
}

Answer 1

I cannot check all because I use a phone at the moment, but here are some quick observations I saw skimming thru the code:

Never call main ()

-It is not defined in C++ what happens if you call main from another function. It is undefined behavior.

You could instead put your while (1) loop in a function and call it instead of main.

Use of C++ Features

-Consider using std::array or std::vector instead of using plain C style arrays.

Formating

-Formatting seem to be off in some places. Add a space between functions and don’t put several instructions on one line.

e.g.

if (win()) {std::cout << "Player 2 wins!\n";break;}
    ++count;

Should become:

if (win()) {
    std::cout << "Player 2 win\n";
    break;
}
++count;

Also you should avoid having to long lines. Get used to break the code after max 80 lines. It has the advantage that you can view two source codes next to each other.

for example make this:

if (pos[w_combinations[i][0]] == pos[w_combinations[i][1]] && pos[w_combinations[i][0]] == pos[w_combinations[i][2]]) 
{
    return true;
    break;
}

Into this:

if (pos[w_combinations[i][0]] == pos[w_combinations[i][1]] 
    && pos[w_combinations[i][0]] == pos[w_combinations[i][2]]) 
{
    return true;
    break;
}

or this

void printMatrix() {
std::cout << "|" ; check(pos[0]) ; check(pos[1]) ; check(pos[2]);std::cout << "\n";
std::cout << "|" ; check(pos[3]);check(pos[4]) ; check(pos[5]);std::cout << "\n";
std::cout << "|" ; check(pos[6]) ; check(pos[7])  ; 

It’s very important for code to be easy to read. You may be only write a program once but most of the time you read it while debugging or maintaining it. If the code is good formatted this task becomes a lot easier.

Avoid redundancy

In you’re while loop you use the same instructions except the messages for player 1 and 2. Make them a function with parameters for the messages. This way you make sure you handle both players the same.

Avoid global variables

Don’t use global variables they make the code hard to maintain. They can lead to obscure bugs. Consider passing them between you’re functions or even better check out OOP and work with a class who stores the state of the Board.

Functions should do one thing

You’re function check () should only return the string not also print it. It should become this:

std::string check (int x)
{ 
    if ( x == 10) {
        return "X|";
    }
    else if (x == 11) {
        return "O|";
    }
    return std::to_string (X)
}

This function now gets called and the string is directly printed. No need to change anything in the call code.

Also consider using switch instead of a long if else chain if there are many cases.

Calls to std::cout

No need to call std::cout several times. Just do this :

std::cout << '|' + check(pos[0]) + check(pos[1]) + check(pos[2]) + '\n';

Single chars should be quoted ' ' not " " to avoid getting a more expensive string use instead of a char.

Handle invalid input

Try to enter something which is not a the expected number. What happens? The program probably loops endless or crashes because you read no int. You should handle this case. One approach would be reading the in put as a std::string and check this string if it is a digit. If it is not prompt an invalid input and let the user enter again.

But wait. you can also break the program if it is an int but not in range. Handle also wrong range. for example when you input 1234 the program will crash currently as well. You could do all this in a function:

int get_input();

So you can write:

int x = get_input();

Use descriptive names

Function names like Check or Input are very mysterious to me about what they are doing.

The same goes for variable names like x, y or w_combinations. Try to self document youre code by using clear names which describe what the variable or function does.

Ask yourself if you check this code in 1 or 2 months yourself if you still can get what the program does. With good naming’s and a clear structure it becomes a lot easier.

Use C++ Features like Classes

Youre code looks very C like. Make more use of C++ features like classes etc. I rewrote the program with classes to demonstrate a more C++ Style as an inspiration (maybe I should also post it for review?). Don't get overwhelmed it takes a lot of time and practice to get into C++...

tic_tac_toe.h

#ifndef TIC_TAC_TOE_020120180815
#define TIC_TAC_TOE_020120180815

#include <array>
#include <string>

namespace tic_tac_toe
{
    class TicTacToe final{ 
    public:
        TicTacToe() = default;
        ~TicTacToe() = default;

        // delete copy and move mechanism, we don't want to 
        // copy a running game
        TicTacToe(const TicTacToe&) = delete;
        TicTacToe(TicTacToe&& other) = delete;
        TicTacToe& operator=(const TicTacToe& other) = delete;
        TicTacToe& operator=(TicTacToe&& other) = delete;

        void print_state_of_board() const;
        bool draw(int field);
        bool board_full() const;
        bool player1_win() const
        {
            return check_win_condition(FieldState::player1);
        }

        bool player2_win() const
        {
            return check_win_condition(FieldState::player2);
        }
    private:
        enum class FieldState {
            empty,
            player1, // X
            player2, // O
        };

        bool check_win_condition(FieldState state) const;
        char field_state_to_char(FieldState state) const;


        std::array<FieldState, 9> m_board{ FieldState::empty };
        bool m_player1_active{ true };

        static constexpr char m_player1_token{ 'X' };
        static constexpr char m_player2_token{ 'O' };
    };

    int get_user_input(const std::string& user_message);

    void play_game();   // main routine to run the game logic;
}  // namespace tic_tac_toe
#endif

tic_tac_toe.cpp

#include "tic_tac_toe.h"

#include <algorithm> // std::find
#include <cctype> // std::stoi
#include <iostream>
#include <vector>

namespace tic_tac_toe
{
    void TicTacToe::print_state_of_board() const
        /*
        Print the board. e.g:
        |X| |O|
        | |X| |
        |O| | |
        */
    {
        for (auto i = 0; i < m_board.size(); ++i) {
            if (i % 3 == 0 && i != 0) {
                std::cout << "|\n";
            }
            auto token = field_state_to_char(m_board.at(i));
            std::cout << '|' << token;
        }
        std::cout << "|\n";
    }

    bool TicTacToe::draw(int field)
        /*
        Tries to draw the next symbol in the field.
        Each time the function is called the player is changed.
        The user input has to be done out side. This way also a bot
        could play the game.
        If the selected field can not be set because its already 
        occupied by player1 or player2 or out of range the return
        value becomes false
        */
    {
        if (field < 1 || field > m_board.size() || 
            m_board.at(field - 1) != FieldState::empty) {
            return false;
        }
        if (m_player1_active) {
            m_board.at(field - 1) = FieldState::player1;
            m_player1_active = false;
        }
        else { // player 2 active
            m_board.at(field - 1) = FieldState::player2;
            m_player1_active = true;
        }
        return true;
    }

    bool TicTacToe::board_full() const
        /*
        search for a empty field in the board
        indicating that board is full if no empty field available.
        */
    {
        auto it = std::find(
            m_board.begin(), m_board.end(), FieldState::empty);

        return it == m_board.end();
    }

    bool TicTacToe::check_win_condition(FieldState state) const
    {
        constexpr std::array<std::array<int, 3>, 8> combinations =
        { 
            std::array<int, 3>{0,1,2},
            std::array<int, 3>{3,4,5},
            std::array<int, 3>{6,7,8},
            std::array<int, 3>{0,3,6},
            std::array<int, 3>{1,4,7},
            std::array<int, 3>{2,5,8},
            std::array<int, 3>{0,4,8},
            std::array<int, 3>{2,4,6}
        };

        for (const auto& combination : combinations) {
            if (m_board.at(combination[0]) == state &&
                m_board.at(combination[1]) == state &&
                m_board.at(combination[2]) == state) {
                return true;
            }
        }       
        return false;
    }

    char TicTacToe::field_state_to_char(FieldState state) const
    {

        if (state == FieldState::player1) {
            return m_player1_token;
        }
        if (state == FieldState::player2) {
            return m_player2_token;
        }
        return ' ';
    }

    int get_user_input(const std::string& user_message)
    {
        while (true) {
            std::cout << user_message;
            std::string input;
            std::cin >> input;
            /*
            If input is not an integer the stoi function will raise
            an exception. We use this to determine if the input was
            an int
            */
            try {
                return std::stoi(input);
            }
            catch (std::invalid_argument&) {
                std::cout << "\nInput is not a number. Try again:";
            }
        }
    }

    void play_game()
        /*
        Main routine to play the game with 2 players
        */
    {
        while (true) {
            TicTacToe game;
            bool player1_active{ true };
            while (!game.board_full() &&
                !game.player1_win() && !game.player2_win()) {

                game.print_state_of_board();

                std::string user_message;
                if (player1_active) {
                    user_message = "1[X]:";
                }
                else {  // player2 active
                    user_message = "2[O]:";
                }
                if (!game.draw(get_user_input(user_message))) {
                    std::cout << "\nInvalid! Try again: \n";
                }
                else {
                    player1_active = !player1_active;
                }
            }

            game.print_state_of_board();

            if (game.player1_win()) {
                std::cout << "Player 1 wins!\n";
            }
            else if (game.player2_win()) {
                std::cout << "Player 2 wins!\n";
            }
            else {
                std::cout << "Draw!\n";
            }

            int choice{};
            while (true) {
                choice = get_user_input(
                    "Play again[Yes = 1, No = 0]: ");

                if (choice == 0) {
                    return;
                }
                if(choice == 1) {
                    break;
                }
            }
        }
    }
}  // namespace tic_tac_toe

main.cpp

#include "tic_tac_toe.h"

#include <iostream>

int main()
try {
    tic_tac_toe::play_game();
}
catch (std::runtime_error& e) {
    std::cerr << e.what() << "\n";
    std::getchar();
}
catch (...) {
    std::cerr << "unknown error " << "\n";
    std::getchar();
}

Answer 2

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

Eliminate global variables where practical

The code declares and uses 3 global variables. Global variables obfuscate the actual dependencies within code and make maintainance and understanding of the code that much more difficult. It also makes the code harder to reuse. For all of these reasons, it's generally far preferable to eliminate global variables in favor of local variables and pass pointers to variables for functions needing them. For instance, the only function using w_combinations is win(), so I would declare it static constexpr and put it within the body of that function.

Eliminate unused variables

Unused variables are a sign of poor code quality, so eliminating them should be a priority. In this code, end is initialized but never used. My compiler also tells me that. You compiler is probably also smart enough to tell you that, if you ask it to do so.

Eliminate spurious statements

Inside the win() function is this strange construction:

{
    return true;
    break;
}

The break is never going to be executed, so it would be better to simply omit it.

Use more whitespace to enhance readability of the code

Instead of crowding things together like this:

if (game.win()) {std::cout << "Player 1 wins!\n";break;}

most people find it more easily readable if you use more space:

if (game.win()) {
    std::cout << "Player 1 wins!\n";
    break;
}

Don't call main

The C++ standard specifically prohibits calling main, so doing so results in undefined behavior, meaning that the compiler could do anything. If you need to have the program loop, write a loop.

Prefer looping to unbounded recursion

Unlike main, it is legal C++ to call input from within the input function. However, in this case, it's not a good idea. The goal here is to keep trying to get input until we get a valid input. Here's a way to write that which uses a loop instead of recursion:

void input(int player) {
    const char *prompt[2] = { "1[X]:", "2[O]:" };
    const char token[2] = { 'X', 'O' };
    assert(player == 0 || player == 1);
    std::cout << prompt[player];
    while (1) {
        int square;
        std::cin >> square;
        --square;
        if (square >= 0 && square < 9 && pos[square] == '1'+square) {
            pos[square] = token[player];
            return;
        }
        std::cout << "\nInvalid! Try again: ";
    }
}

Note that player is now 0 or 1 instead of 1 or 2 and that I've used characters for pos instead of int. More on that later.

Use objects

The game is written much more in the procedural style of C rather than in the object-oriented style of C++. The game itself could be an object, with most of the procedures as functions of that object. This would reduce coupling and make the program easier to understand. It would also easily eliminate the global variables that currently exist in the code.

Use better variable and function names

The variable name w_combinations is ok, but the name check() is not. The first name explains something about what the variable means within the context of the code, but the latter is only confusing. A better name might be printSquare().

Rethink the interfaces

The printMatrix() code currently looks like this:

void printMatrix() {
    std::cout << "|" ; check(pos[0]) ; check(pos[1]) ; check(pos[2]);std::cout << "\n";
    std::cout << "|" ; check(pos[3]);check(pos[4]) ; check(pos[5]);std::cout << "\n";
    std::cout << "|" ; check(pos[6]) ; check(pos[7])  ; check(pos[8]); std::cout << "\n";
}

It relies on a function called check.

void check (int x) { 
    if ( x == 10) {
        std::cout << "X|";
    }
    else if (x == 11) {
        std::cout << "O|";
    }
    else {
        std::cout << x << "|";
    }
}

This could be made much less verbose by doing two things: first, use char instead of int for pos and second, use the values X and O instead of 10 and 11. Then it could be written like this:

void printMatrix() {
    std::cout 
        << '|' << pos[0] << '|' << pos[1] << '|' << pos[2] << '\n'
        << '|' << pos[3] << '|' << pos[4] << '|' << pos[5] << '\n'
        << '|' << pos[6] << '|' << pos[7] << '|' << pos[8] << '\n';
}

Alternatively, one could write a loop.

Reduce redundant code

Each player's turn is essentially the same code, with only the prompt changing, so it would make sense to write it that way. Using all of the suggestions above, here's what my rewritten main looks like:

int main() {
    bool playing = true;
    while (playing) {
        TicTacToe game;
        game.play();
        std::cout << "Play again[Yes = 1, No = 0]: ";
        int permission;
        std::cin >> permission;
        playing = (permission == 1);
        std::cout << "\n\n";
    }
}

Notice that everything is wrapped up in a TicTacToe game object which has play() as its only public member function. There are, of course, other ways to do this, but I hope that whets your appetite for learning more C++.