First Program Tic-Tac-Toe

Question

This is the first little project I've made that didn't feel it was complete gibberish. But I couldn't tell.

The biggest problem I had with this was using the BoardValue enum working like I wanted to. I understand that classes should have a level of abstraction to them and I suspect that the way I implemented the at(int) returning a char over a BoardValue took away from that. However, I though having to convert the return from at(int) to a char if it returned a BoardValue would be redundant. For example, using a statement like this:

char print_char = Board.at(some_index) == BoardValue::o ? 'O' : 'X';

I hope I've done a decent job describing my dilemma.

Overall, I'm hoping for some overall general code style tips and pointers on how to write better code from here.

tictactoe.h

#ifndef TICTACTOE
#define TICTACTOE
#include <array>
#include <iostream>

enum BoardValue : char{ none = ' ', o = 'O', x = 'X' };

class Board
{
public:
    Board()
    {
        for(auto begin = board.begin(),end = board.end();begin != end; ++begin)
            *begin = BoardValue::none;
    }

    char at(int index) const{ return board.at(index); }
    inline bool check_win(BoardValue) const;
    bool place(int, BoardValue);
private:
    bool check_diagonals(BoardValue) const;
    bool check_horizontals(BoardValue) const;
    bool check_verticals(BoardValue) const;

    std::array<char, 9> board{};
};

inline bool Board::check_win(BoardValue check) const
{
    if(check == BoardValue::none)
        throw "Trying to check_win for check == BoardValue::none!";
    return check_diagonals(check) || check_horizontals(check) || check_verticals(check);
}

#endif

tictactoe.cpp

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

//returns false if index is occupied
bool Board::place(int index, BoardValue value)
{
    if(board.at(index) != BoardValue::none)
        return false;
    board.at(index) = value;
    return true;
}

bool Board::check_diagonals(BoardValue check) const
{
    //if middle is not check no diagnols will pass
    if(board.at(4) != check)
        return false;
    //backward diagonal '\'
    if(board.at(0) == check && board.at(4) == check)
        return true;
    //forward diaganol '/'
    if(board.at(2) == check && board.at(6) == check)
        return true;
    return false;
}

bool Board::check_horizontals(BoardValue check) const
{
    for(int row = 0; row < 3; ++row){
        if(board.at(row) == check &&
            board.at(row + 3) == check &&
            board.at(row + 6) == check)
            return true;
    }
    return false;
}

bool Board::check_verticals(BoardValue check) const
{
    for(int col = 0; col < 3; ++col){
        if(board.at(col * 3) == check &&
            board.at(col * 3 + 1) == check &&
            board.at(col * 3 + 2 ) == check)
            return true;
    }
    return false;
}

main.cpp

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

int ask_input(char player, bool retry = false)
{
    if(!retry)
        std::cout << "It's " << player
            << "'s turn. Where do you want to go(e.g. A1 B3 C2)? ";
    else
        std::cout << "No, no, no " << player
            << "! Input a letter followed bt a number: ";
    std::string input;
    std::cin >> input;

    if(input.size() < 2)
        return ask_input(player, true);

    int col_input{};
    switch(*input.begin())
    {
        case 'A':
        case 'a':
            col_input = 0;
            break;
        case 'B':
        case 'b':
            col_input = 1;
            break;
        case 'C':
        case 'c':
            col_input = 2;
            break;
        default:
            return ask_input(player, true);
    }

    int row_input = *(input.begin() + 1) - '0'; //convers char '1' to int 1
    --row_input;

    return col_input * 3 + row_input;
}

BoardValue ask_turn() //ask whos first if return true O goes first
{
    BoardValue turn;
    std::string input;
    std::cout << "Who goes first(X or O)? ";
    for(bool valid_input{false}; !valid_input;)
    {
        std::cin >> input;
        switch(input.front()) //input cannot be null at this point
        {
            case 'x':
            case 'X':
                valid_input = true;
                turn = BoardValue::x;
                break;
            case '0':
            case 'o':
            case 'O':
                valid_input = true;
                turn = BoardValue::x;
                break;
            default:
                std::cout << "Invalid input! Try X or O :";
        }
    }
    return turn;
}

std::ostream &print_board(std::ostream &os,const Board &board)
{
    os << " |A|B|C\n";
    for(int row = 0; row < 3; ++row)
    {
        os << std::string( 8, '-') << '\n';
        os << row + 1 << '|';
        for(int col = 0; col < 3; ++col)
        {
            char follow_char{ col == 2 ? '\n' : '|' };
            os << board.at(col * 3 + row) << follow_char;
        }
    }
    os << std::endl;
    return os;
}

int main(){
    Board board{};
    BoardValue turn{ ask_turn() }; 
    //turn will be set back to appropriate value at start of game loop
    turn = turn == BoardValue::o  ? BoardValue::x : BoardValue::o;
    int turn_count{0};
    while(board.check_win(turn) == false)
    {
        turn = turn == BoardValue::o  ? BoardValue::x : BoardValue::o;
        print_board(std::cout, board);
        bool input_valid{false};
        while(input_valid == false)
        {
            int input;
            input = ask_input(turn);
            input_valid = board.place(input, turn);
            if( input_valid == false )
                std::cout << "That place is take! Try again..\n";
        }
        if(++turn_count == 9) //max amount of turns game is tie
            break;
    }
    print_board(std::cout, board);
    if(turn_count == 9)//game is tie
        std::cout << "Looks like its a tie...\n";
    else
        std::cout << (char)turn << " wins!\n";
}

Answer 1

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

Use the required #includes

The code uses std::string which means that it should #include <string>. It was not difficult to infer, but it helps reviewers if the code is complete.

Have you run a spell check on comments?

If you run a spell check on your comments, you'll find a number of things such as "diagnols" and "diaganol" instead of "diagonals" and "diagonal." Since your code is nicely commented, it's worth the extra step to eliminate spelling errors.

Be wary of recursive calls

The ask_input routine has a subtle flaw. In particular, because it is recursive, it may be possible for a malicious user to crash the program by exhausting the stack. All that would be required would be to continue to input improperly formatted data. For this reason, as well as to make the code more understandable, I'd suggest instead to make retry a local variable and use that, as in a while loop, to re-ask if needed.

Fix the bug

The ask_input has a not-so-subtle flaw as well. It checks the letter, but not the number, so a user could input C9 or A0 and the program would attempt to use that!

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Be judicious with the use of inline

If a function is small and time critical, it makes sense to declare it inline. However, the check_win function is not really time critical, so I would say that there's no reason to make it inline.

Consider using a stream inserter

The existing print_board function is written exactly as one would write as one would write a stream inserter. The only thing that would change would be the declaration:

std::ostream &operator<<(std::ostream& os, const Board& board) { /* ... */ }

Simplify your constructor

The Board constructor is currently defined like this:

Board()
{
    for(auto begin = board.begin(),end = board.end();begin != end; ++begin)
        *begin = BoardValue::none;
}

There are at least three ways to simplify it. One would be to use a "range-for" syntax:

Board()
{
    for(auto& space : board) {
        space = BoardValue::none;
    }
}

Another would be use fill:

Board() {
    board.fill(BoardValue::none);
}

A third way would allow you omit the constructor entirely. Do that by using aggregate initialization in the declaration of board:

std::array<char, 9> board{
    ' ',' ',' ',
    ' ',' ',' ',
    ' ',' ',' ',
};

Think carefully about the class design

The structure of the code is not bad, but some things to think about are what things should be the responsibility of the Board class and which are not. For example, I think it might make more sense for Board to keep track of the number of turns.

Simplify the code

This line is not easy to read or understand:

turn = turn == BoardValue::o  ? BoardValue::x : BoardValue::o;

I would suggest instead having turn be a bool that represents O. Then flipping back and forth would simply be turn = !turn;.

Answer 2

In addition to Edwards answer, there is a bug in check_diagonals. The first check for the '\' diagonal should check for positions 0 and 8.

I think you also switched up the names for check_horizontal and check_vertical, since check_vertical effectively checks the rows and check_horziontal checks columns.

Answer 3

This is a code inspection so it's my role to raise questions, not to answer them. I haven't run your code. Did you check your end condition? It appears to me that the first player, let's say it's x, goes on turn_count 0, 2, 4, 6, 8. In the loop when turn_count is 8 you will accept input from x, place it on the board, then increment turn_count to 9 and break out of the loop. The end condition then checks that count is nine and concludes it's a tie. Thus any game that fills the board is classed a draw, without checking it.

The best solution for that is to move the turn count check into the while condition, testing it second, and saving the result of the win checking in a variable for testing outside the loop.

You can also turn the while-do into a do-while since neither a win nor an exceeded turn count can occur at the beginning.

Then make sure you test a game with a player winning with a full board.

And please, even if you can't spell-check your comments as the current best answer suggests, at the very least ensure that all your printed output is spelled correctly! If you start working for a company producing real code those typos are simply an embarrassing proof that the code was never reviewed or thoroughly tested.

Answer 4

A company I used to work for asked candidates to code a quick Tic Tac Toe implementation as part of the interview process. We used these to sanity check a candidate's basic ability to code. Based on that experience, I have two pieces of general feedback.

1. Stylistically, this code strikes me as workable but a bit windy / verbose. You're using "object orientation" but there's no real sophistication in the OOP, nor any need for it over such a simple domain, so your objects are just containers with friendly names. You're writing explicit code to check columnar and row state (CheckVerticals, CheckHorizontals, CheckDiagonals) which is easily normalized. This code may work, but it's not a joy to read and doesn't seem to have a cohesive shape beyond OOP-by-default. That said, it's still better than the majority of TTT samples I've looked at.

2. What would give your code a more cohesive shape? One way would be: rewrite your code using bitwise operations to represent board state and detect win conditions. This would shorten and tighten your logic, and in particular, those cumbersome explicit checks for various win conditions would melt away.

All in all, your code is good enough I would feel comfortable, in a formal code review, pushing you to produce something tighter and a bit more opinionated. If you can produce the above code, you can produce the above code with tighter logic.

Answer 5

Some little things not mentioned yet:

If you are using C++11 or higher consider using the more safe enum class instead of the plain enum inherited from C. See: https://stackoverflow.com/questions/18335861/why-is-enum-class-preferred-over-plain-enum

Always use Brackets. Its more safe. See: https://softwareengineering.stackexchange.com/questions/16528/single-statement-if-block-braces-or-no

This:

std::ostream &print_board(std::ostream &os,const Board &board)

Should be formated like this:

std::ostream& print_board(std::ostream& os,const Board& board)

Atleast in the C++ style its more common to add Pointer * or reference & to the type, not to the variable name (In C code the other is more common).