8
\$\begingroup\$

I'm looking for any feedback I can get about my tic tac toe game in C++. I don't know what flaws to look for in code or design so that's why I come here to ask. My goal is to improve and write "better" code whatever that means.

MAIN.CPP:

#include <iostream>
#include "game.h"
#include "ui.h"
#include "scoregame.h"
#include "ai.h"

int main(){
    TicTacToe::Game tictac;

    char c = 0;
    TicTacToe::Player player = TicTacToe::Player::x;
    std::cout<<"\nPlay as x or o? ";
    std::cin>>c;
    if(c == 'o' || c == 'O')
        player = TicTacToe::Player::o;
    else if( c == 'x' || c == 'X')
        player = TicTacToe::Player::x;
    else
        std::cout<<"Invalid choice. Picking X by default. ";

    while(TicTacToe::scoreGame(tictac) == TicTacToe::Winner::game_in_progress){
        TicTacToe::printGame(tictac);
        if(tictac.getTurn() == player){
            unsigned move = TicTacToe::promptSquare();
            if(! tictac.makeMark(move) )
                std::cout<<"Illegal move!"<<std::endl;
        }
        else
        {
            tictac.makeMark( TicTacToe::aiMark(tictac) );
        }
    }
    TicTacToe::printGame(tictac);

    std::cout<<std::endl;
    TicTacToe::Winner winner = TicTacToe::scoreGame(tictac);

    if( ((winner == TicTacToe::Winner::x) && (player == TicTacToe::Player::x)) || ((winner == TicTacToe::Winner::o) && (player == TicTacToe::Player::o)) )
        std::cout<<"You won!"<<std::endl;

    else if (winner == TicTacToe::Winner::draw)
        std::cout<<"Draw!"<<std::endl;
    else
        std::cout<<"You lose :("<<std::endl;

    std::cout<<std::endl;
    return 0;
}

PLAYER.H:

#pragma once

namespace TicTacToe {
    enum class Player {x, o, none};
}

BOARD.H:

#pragma once

#include <cstdint>
#include "player.h"

namespace TicTacToe{
    class Board{
        public:
        void clear (); // Set all squares to no marker
        void markSquare (unsigned square, Player marker); // Unconditionally set square to marker
        Player getMark (unsigned square) const;

        private:
        // pattern is divided into 2-bits segments. 0: marked by none. 1: marked by x. 2: marked by o.
        // The 2 least significant bits encode square 1.
        // The Following 2 bits represent square 2, and so forth.
        uint32_t board = 0;
    };

}

BOARD.CPP:

#include "board.h"


namespace TicTacToe{

    void Board::clear(){
        board = 0;
    }

    void Board::markSquare (unsigned square, Player marker){
        uint32_t mask = 0;
        switch(marker){
            case Player::none:
                mask = 0;
                break;
            case Player::x:
                mask = 1;
                break;
            case Player::o:
                mask = 2;
                break;
        }
        mask <<= ((square-1)*2);
        board &= ~mask;
        board |= mask & 0x3FFFF; // Only allow 9 squares
    }

    Player Board::getMark (unsigned square) const {
        uint32_t mask = board >> ((square-1)*2) & 3;
            if(mask == 1)
                return Player::x;
            if(mask == 2) 
                return Player::o;
            return Player::none;
    }

}

GAME.H:

#pragma once

#include "board.h"

namespace TicTacToe{

    class Game{
        public:
        void reset();
        bool makeMark (unsigned square); // Current player marks square. Turn is flipped. False on illegal move.
        Player getMark(unsigned square) const;

        Player getTurn () const;

        private:
        Player turn = Player::x;
        Board board;
    };

}

GAME.CPP:

#include "game.h"

namespace TicTacToe{


    void Game::reset(){
        board.clear();
        turn = Player::x;
    }

    bool Game::makeMark (unsigned square){
        if(board.getMark(square) != Player::none)
            return false;

        board.markSquare(square, turn);

        if(board.getMark(square) != turn)
            return false;

        if(turn == Player::x)
            turn = Player::o;
        else
            turn = Player::x;

        return true;
    }

    Player Game::getMark(unsigned square) const {
        return board.getMark(square);
    }

    Player Game::getTurn () const {
        return turn;
    }


}

UI.H:

#include "game.h"

namespace TicTacToe{
    void printGame(const Game& game);
    unsigned promptSquare();
}

UI.CPP:

#include "ui.h"
#include <limits>
#include <iostream>

namespace TicTacToe{
    void printGame(const Game& game){
        std::cout<<std::endl;
        for(int s=1; s <= 9; ++s){
            if((s-1)%3 == 0)
                std::cout<<std::endl;
            switch(game.getMark(s)){
                case Player::x:
                    std::cout<<"X";
                    break;
                case Player::o:
                    std::cout<<"O";
                    break;
                default:
                    std::cout<<"?";
            }
        }
        std::cout<<std::endl;
    }

    unsigned promptSquare(){
        std::cout<<std::endl<<"Your turn (1-9): ";
        unsigned in;
        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(),'\n');
        std::cin>>in;
        return in;
    }

}

SCOREGAME.H:

#pragma once

#include "game.h"

namespace TicTacToe{

    enum class Winner {game_in_progress, x, o, draw};
    Winner scoreGame (const Game & ); 

}

SCOREGAME.CPP:

#include "scoregame.h"

namespace TicTacToe {
    Winner scoreGame (const Game & game){
        Player winner = Player::none;

        for(int s=0; s < 3; ++s){
            Player column_who = game.getMark(s+1);
            if(column_who != Player::none && column_who == game.getMark(s+4) && column_who == game.getMark(s+7))
                winner = column_who;

            int row_start = 1+s*3;
            Player row_who = game.getMark(row_start);
            if(row_who != Player::none && row_who == game.getMark(row_start+1) && row_who == game.getMark(row_start+2))
                winner = row_who;
        }

        Player mid_who = game.getMark(5);

        if(mid_who != Player::none && mid_who == game.getMark(1) && mid_who == game.getMark(9))
            winner = mid_who;

        if(mid_who != Player::none && mid_who == game.getMark(3) && mid_who == game.getMark(7))
            winner = mid_who;

        if(winner == Player::x)
            return Winner::x;

        if(winner == Player::o)
            return Winner::o;

        for(int i=1; i <= 9 && game.getMark(i) != Player::none; ++i)
            if(i == 9)
                return Winner::draw;

        return Winner::game_in_progress;
    }   
}

AI.H:

#include "game.h"

namespace TicTacToe{

    unsigned aiMark(Game&);
}

AI.CPP:

#include <iostream>
#include "scoregame.h"

namespace TicTacToe{

        static int minimax (Game g, unsigned & move) {

            switch(scoreGame(g)){
                case Winner::draw:
                    return 0;
                case Winner::x:
                    return 1;
                case Winner::o:
                    return -1;
                default: break;
            }


            int best_score = g.getTurn() == Player::o ? 2 : -2;
            int best_move = 0;

            for(int m=1; m <= 9; ++m){
                Game next(g);
                if(!next.makeMark(m))
                    continue;
                int next_score = minimax(next, move);
                if( (g.getTurn() == Player::o && next_score < best_score) || (g.getTurn() == Player::x && next_score > best_score) )
                {
                    best_score = next_score;
                    best_move = m;
                }
            }
            move = best_move;
            return best_score;
        }

        // Return 0 if no move is available
        unsigned aiMark(Game& g) {
            if( scoreGame(g) != Winner::game_in_progress )
                return 0;
            unsigned move;
            minimax(g, move);
            return move;            
        }

}
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

This is some really well-written code! It's separated out very nicely.

Namespaces

It's great that you used namespaces. In general, I don't tend to complain about long names. But I did find it a bit tedious to read some of the code because of all of the scoping. You could get rid of at least one level of it by adding a using namespace TicTacToe in main.cpp. It just seem a bit verbose to have to write out TicTacToe::Player::x just to get x out of it.

Simplify Your Conditional Blocks

In main(), you have a conditional block which runs the game:

    if(tictac.getTurn() == player){
        unsigned move = TicTacToe::promptSquare();
        if(! tictac.makeMark(move) )
            std::cout<<"Illegal move!"<<std::endl;
    }
    else
    {
        tictac.makeMark( TicTacToe::aiMark(tictac) );
    }

This could be simplified to only call tictac.makeMark() once by reformatting it like this:

unsigned move;
if (tictac.getTurn() == player) {
    move = TicTacToe::promptSquare();
}
else
{
    move = TicTacToe::aiMark(tictac);
}

if (!tictac.makeMark(move)) {
    std::cout << "Illegal move" << std::endl;
}

Presumably aiMark() can never return a bad move, so the block inside the if will never execute on an ai move.

Simplify Your Board

You've chosen to make the board fit into a single uint32_t and to index it starting at 1 rather than 0. These 2 things complicate your code significantly. It would be better, in my opinion, to make it an actual 3 x 3 array of Player objects. (It might be better to rename Player to Move, Mark or some other name that fits this use better.)

By doing this, you eliminate all code that involves masking. You eliminate all code where you have to subtract 1 from the loop value. And you make the code easier to understand. I doubt you'll notice any performance difference, either.

It does mean that instead of a single index for square (for example in markSquare), you'll need 2 indices - a row and a column. But the tradeoff seems worth it. You also make the user interface simpler for the human player, as figuring out a single index for the squares seems much harder than entering a row and column. (You could even use the chess method of labeling things, where columns are lettered and rows are numbered.)

Avoid Magic Numbers

In scoreGame you have several places where your adding or subtracting what seem like random values to the index. I would make these into named constants. Something like:

const int kNextRowIncrement = 4;
const int kLastRowIncrement = 7;

But as I mentioned above, you don't need these if you use a proper 2D array. You can simply have nested loops that count over rows and columns.

AI

I just want to say that I'm impressed that you went to the trouble of writing a minimax solver for this. It's much easier to brute-force a reasonable next move in a simple game like this, but you probably learned more by going the minimax route. Well done!

\$\endgroup\$
2
  • \$\begingroup\$ How can you brute-force the next move? \$\endgroup\$
    – tictactoe
    Aug 28, 2017 at 17:15
  • \$\begingroup\$ Basically take the center square if it's not already taken, since that allows you 4 ways to win, whereas all other squares allow only 2 or 3. Then once that's gone simply try each square to see if you can win. If so, take that square. If not, check each square to see if your opponent will win, and if so, block them. If no squares can win for you and none can win for your opponent, just pick the first one you come across. \$\endgroup\$ Aug 29, 2017 at 1:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.