6
\$\begingroup\$

I made a Tic Tac Toe game using classes to show a better approach than the Tic Tac Toe game in Tic Tac Toe in C++11 which works not with classes.

I already posted the code there as an improvement over the code posted in that question.

Now I wonder what I can still improve on my solution?

Feel free to comment on anything which comes to youre mind to improve this code.

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();
}
\$\endgroup\$
  • 2
    \$\begingroup\$ What is your reason to delete copy and move constructor and assignment? I don’t see a need, as the class owns no data or resources, copy and move would be trivial, let the compiler generate those for you if needed, if you don’t use them they won’t be generated. \$\endgroup\$ – Cris Luengo Jan 5 at 20:13
  • \$\begingroup\$ hm i thought they are generated by default if you dont declare them. And since it makes no sense to copy a running game i thought its better to change from default to delete. \$\endgroup\$ – Sandro4912 Jan 5 at 21:34
5
\$\begingroup\$

This is definitely much improved over the previous version. Good job! There may still some things that might be improved.

Use only necessary #includes

The #include <vector> line is not necessary and can be safely removed.

Use all required #includes

Because main refers to std::runtime_error it should have #include <stdexcept>.

Reduce runtime complexity where practical

The std::find used in board_full is not bad, and as a practical matter, no human will ever notice the difference between it and a faster mechanism, but the simpler method is to simply keep a turn counter. If the number of turns is 9, then either someone just won or it's a tie. That would reduce the code to this:

bool isTie() const {
    return turn_count >= 9;
}

Note also that I've used the very common is prefix to make it perfectly unambiguous that it's a function returning a bool and isTie make clear what true means.

Also, the play_game is more complex than it needs to be. Here's an alternative version:

void play_game() {
    TicTacToe game;
    game.play();
}

void TicTacToe::play() {
    for (bool ingame = true; ingame; player = 1 - player) {
        printMatrix();
        input(player);
        if (isWin()) {
            std::cout << "Player " << player+1 << " wins!\n";
            ingame = false;
        } else if (isTie()) {
            std::cout << "Draw!\n";
            ingame = false;
        }
    }
}

In this version, the game keeps track of which player is playing, rather than having that knowledge external to the object.

Use object to group related things

In various places within the code, there is a bool player1_active, prompts such as "1[X]:", m_player1_token and FieldState::player1. I think it would make the code simpler if these concepts were all grouped together into a Player object. This would also make it quite simple to adapt a robot player, since it could likely use the same Player interface, if carefully designed.

Think about the user

Although I think the board is less cluttered and easier to read in this version than when it had the squares numbered, it makes it difficult for the user to figure out how to enter a move. Having some instructions or maybe statically printing the numbering scheme as part of the prompt might help. Also, when one player has won the game it says something like "Player 2 wins!" but what the user deals with and thinks about is "X" and "O", not "1" and "2".

Example result

Here's how all of that might look if implemented.

tic_tac_toe.h

#ifndef TIC_TAC_TOE_H
#define TIC_TAC_TOE_H

#include <array>
#include <string>

namespace tic_tac_toe
{
    class TicTacToe;

    class Player {
    public:
        constexpr Player(char token, const char* winmsg, const char* prompt) : 
            token{token}, 
            winmsg{winmsg},
            prompt{prompt}
        {}
        int select(const TicTacToe &board) const; 
        const char token;
        const char *winmsg;
        const char *prompt;
    };

    class TicTacToe final { 
    public:
        TicTacToe() {
            m_board.fill(emptyToken);
        }
        void play(); 
        bool occupied(unsigned square) const {
            return square >= num_squares || m_board[square] != emptyToken;
        }
        static constexpr int num_squares{9};
        static constexpr char emptyToken{' '};
    private:
        void print_state_of_board() const;
        bool isWin() const;
        void input(int player);
        bool isTie() const {
            return turn_count >= num_squares;
        }

        static constexpr Player players[2] = { 
            { 'X', "X Wins!", "1[X]:" },
            { 'O', "O Wins!", "2[O]:" },
        };
        int turn_count = 0;
        int player = 0;
        std::array<char, num_squares> m_board;
    };

    int get_user_input(const std::string& user_message);
    void play_game();   
}
#endif

tic_tac_toe.cpp

#include "tic_tac_toe.h"
#include <cctype> 
#include <iostream>

namespace tic_tac_toe
{
    constexpr Player TicTacToe::players[2];
    constexpr char TicTacToe::emptyToken;

    int Player::select(const TicTacToe &board) const {  
        while (true) {
            int answer = get_user_input(prompt)-1;
            if (!board.occupied(answer)) 
                return answer;
            std::cout << "Invalid input; try again\n";
        } 
        return 0;  // should never get here!
    }

    void TicTacToe::print_state_of_board() const {
        auto col{3};
        for (std::size_t i = 0; i < m_board.size(); ++i) {
            std::cout << '|' << m_board.at(i);
            if (--col == 0) {
                std::cout << "|\n";
                col = 3;
            }
        }
    }

    bool TicTacToe::isWin() const {
        static constexpr std::array<std::array<int, 3>, 8> combinations{{ 
                {0,1,2}, {3,4,5}, {6,7,8}, {0,3,6},
                {1,4,7}, {2,5,8}, {0,4,8}, {2,4,6}
        }};
        for (const auto& combination : combinations) {
            if (m_board.at(combination[0]) == players[player].token &&
                    m_board.at(combination[1]) == players[player].token &&
                    m_board.at(combination[2]) == players[player].token) 
            {
                return true;
            }
        }       
        return false;
    }

    void TicTacToe::input(int player) {
        m_board[players[player].select(*this)] = players[player].token;
        ++turn_count;
    }
    void TicTacToe::play() {
        for (bool ingame = true; ingame; player = 1 - player) {
            print_state_of_board();
            input(player);
            if (isWin()) {
                std::cout << players[player].winmsg << '\n';
                ingame = false;
            } else if (isTie()) {
                std::cout << "Draw!\n";
                ingame = false;
            }
        }
    }

    int get_user_input(const std::string& user_message)
    {
        while (true) {
            std::cout << user_message;
            std::string input;
            std::cin >> input;
            try {
                return std::stoi(input);
            }
            catch (std::invalid_argument&) {
                std::cout << "\nInput is not a number. Try again:";
            }
        }
    }

    void play_game() {
        for (bool gaming{true}; 
            gaming; 
            gaming = get_user_input("Play again[Yes = 1, No = 0]: ") == 1)
        {
            TicTacToe game;
            game.play();
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback. Im not sure of how to implement the Player interface. How can std::array<FieldState, 9> m_board{ FieldState::empty } By making 3 Player Objects in the TicTacToe class? I would appreachiate an example of how the Player interface could look. \$\endgroup\$ – Sandro4912 Jan 5 at 10:30
  • \$\begingroup\$ I've updated the answer to show a fully worked version. \$\endgroup\$ – Edward Jan 5 at 18:32
  • \$\begingroup\$ thanks for the example. Some questions about it. Is there a reason to use char* for the literals? Ithought in c++ you should avoid it. also in youre cpp i dont think you need constexpr Player TicTacToe::players[2]; constexpr char TicTacToe::emptyToken; \$\endgroup\$ – Sandro4912 Jan 6 at 18:03
  • \$\begingroup\$ Good question. The only reason to use char * is to allow the use of constexpr. If your compiler supports std::string_view, that's another way to do it. The limitation all of this is intended to get around is that one can't have a constexpr std::string. The values don't need to be constexpr but it's nicer if they are since it means there is zero runtime overhead and many more compiler optimizations are enabled with constexpr. \$\endgroup\$ – Edward Jan 6 at 20:13
  • \$\begingroup\$ thanks for the hint with string_view. i catched myself replacing const char* in a c++ project with string_view. \$\endgroup\$ – Sandro4912 Jan 7 at 18: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.