First Console Game: Tic-Tac-Toe

Question

I have just finished my first true console game using C++. I would love if I could get some feedback on my program, I have a few specific comments in my code that I have questions on and will list a few more that I ran into while building this game.

1. Does main.cpp have enough data abstraction/any comments on my abstraction, where it could use improvement?
2. As a new programmer, I have most likely overlooked some things that may be general design flaws, so any general criticisms you could give me that would improve my program would be much appreciated as well as personal preferences with an explanation as to an improvement would be very appreciated.
3. In my Board.cpp file, I left a comment about how to structure my conditionals to test for winning combinations.
4. In my Board.hpp file, I left a comment about trying to use the a vector from the STL, however when I try to implement a vector instead of the array I get an error. I copied my code for the vector into my main.cpp file and it worked fine, so I am not sure what I am overlooking on this one.
5. If you could comment on the structure of my Board.hpp file, specifically is the organization of my member functions appropriate? Where could it use work? I also would like to know if I should have created a separate player class and file? Also any comments on why or why not to add an additional class.

main.cpp

//
//  Tic-Tac-Toe
//

#include "Board.hpp"
#include <iostream>
#include <string>

using namespace std;

int main() {
    bool newgame{true};
    while(newgame == true) {
        Board board;
        board.reset_winner();
        string username,playAgain;

        //Select player gamepieces.
        cout << "Enter player X username: ";
        cin >> username;
        board.set_playerX(username);
        cout << "Enter player O username: ";
        cin >> username;
        board.set_playerO(username);

        cout << "\n\nPlayer X: " << board.get_playerX() << "     " << "Player O: " << board.get_playerO() << endl;
        cout << "\nPlayers select a game space 1 - 9 to select a play." << endl;
        board.displayGrid();

        //Gameplay
        while (board.get_isWinner() == false) {
            int selection{0};

            //Player X's move.
            board.set_validSelection(false);
            while (board.get_validSelection() == false) {
                cout << board.get_playerX() << "'s move:";
                cin >> selection;
                board.selection_playerX(selection);
            }
            board.display();
            board.winOrTie('X');

            //Player O's move.
            if (board.get_isWinner() == false) {
                board.set_validSelection(false);
                while (board.get_validSelection() == false) {
                    cout << board.get_playerO() << "'s move:";
                    cin >> selection;
                    board.selection_playerO(selection);
                }
                board.display();
                board.winOrTie('O');
            }
        }
        //Announce winner.
        if (board.get_winner() == 'X') {
            cout << board.get_playerX() << " is the winner!" << endl;
        }
        else if (board.get_winner() == 'O') {
            cout << board.get_playerO() << " is the winner!" << endl;
        }
        else if (board.get_winner() == 'T') {
            cout << "Game ends in a tie." << endl;
        }
        //Play again?
        cout << "Would you like to play again?" << "\nType: \"Y\" or \"N\"" << endl;
        cin >> playAgain;
        if (cin.fail()) {
            cin.clear();
            cout << "Please type \"Y\" or \"N\":" << endl;
            cin >> playAgain;
        }
        else if (cin.good()) {
            if (playAgain == "Y" || playAgain == "y") {
                newgame = true;
            }
            else if (playAgain == "N" || playAgain == "n") {
                newgame = false;
            }
            else {
                newgame = false;
            }
        }
    }

    return(0);
}

Board.hpp

//
//  Board.hpp
//  tictactoe
//

#ifndef Board_hpp
#define Board_hpp

#include <stdio.h>
#include <string>
#include <vector>

#endif /* Board_hpp */

class Board {
private:
    std::string playerX;
    std::string playerO;
    char game_space[10]{'\0',' ',' ',' ',' ',' ',' ',' ',' ',' '}; //Why wont         std::vector<char> game_space(10,' ') work here? *Note: I did try adding #include   <vector> too.
    bool isWinner{false};
    bool validSelection{true};
    char winner;
protected:
public:
    //Player info
    void set_playerX(std::string username);
    void set_playerO(std::string username);
    std::string get_playerX();
    std::string get_playerO();
    //Board info
    void displayGrid();
    void display();
    void selection_playerX(int);
    void selection_playerO(int);
    //Game info
    bool get_isWinner();
    void set_isWinner(bool);
    bool get_validSelection();
    void set_validSelection(bool);
    void winOrTie(char);
    char get_winner();
    void reset_winner();

};

Board.cpp

//
//  Board.cpp
//  tictactoe
//

#include "Board.hpp"
#include <iostream>

//Player Info
void Board::set_playerX(std::string username) {
    playerX = username;
}
void Board::set_playerO(std::string username) {
    playerO = username;
}
std::string Board::get_playerX() {
    return playerX;
}
std::string Board::get_playerO() {
    return playerO;
}
//Board info
void Board::displayGrid() {
    std::cout << std::endl;
    std::cout << "          +---+---+---+" << std::endl;
    std::cout << "          | 1 | 2 | 3 |" << std::endl;
    std::cout << "          +---+---+---+" << std::endl;
    std::cout << "          | 4 | 5 | 6 |" << std::endl;
    std::cout << "          +---+---+---+" << std::endl;
    std::cout << "          | 7 | 8 | 9 |" << std::endl;
    std::cout << "          +---+---+---+" << std::endl;
    std::cout << std::endl;
}
void Board::display() {
    std::cout << std::endl;
    std::cout << "          +---+---+---+" << std::endl;
    std::cout << "          | " << game_space[1] << " | " << game_space[2] << " | " << game_space[3] << " |" << std::endl;
    std::cout << "          +---+---+---+" << std::endl;
    std::cout << "          | " << game_space[4] << " | " << game_space[5] << " | " << game_space[6] << " |" << std::endl;
    std::cout << "          +---+---+---+" << std::endl;
    std::cout << "          | " << game_space[7] << " | " << game_space[8] << " | " << game_space[9] << " | " << std::endl;
    std::cout << "          +---+---+---+" << std::endl;
    std::cout << std::endl;
}
void Board::selection_playerX(int selection) {
    if (game_space[selection] == ' ') {
        game_space[selection] = 'X';
        validSelection = true;
    }
    else {
        std::cout << "Board position already taken." << std::endl;
        validSelection = false;
    }
}
void Board::selection_playerO(int selection) {
    if (game_space[selection] == ' ') {
        game_space[selection] = 'O';
        validSelection = true;
    }
    else {
        std::cout << "Board position already taken." << std::endl;
        validSelection = false;
    }
}
//Gameplay
bool Board::get_isWinner() {
    return isWinner;
}
void Board::set_isWinner(bool kanyewest) {
    isWinner = kanyewest;
}
bool Board::get_validSelection() {
    return validSelection;
}
void Board::set_validSelection(bool selection) {
    validSelection = selection;
}
void Board::winOrTie(char gamepiece) {
    bool fullBoard{false};
    /* 1 2 3
       4 5 6
       7 8 9 */
    //Vertical Win
    if ((game_space[1] == gamepiece) && (game_space[4] == gamepiece) && (game_space[7] == gamepiece)) {
        //Question: Why wont "if ((game_space[1] && game_space[4] && game_space[7]) == gamepiece)" work as a conditional?
        //Also is there a better way to go about the conditionals presented here for combination testing?
        isWinner = true;
        winner = gamepiece;
}
    else if ((game_space[2] == gamepiece) && (game_space[5] == gamepiece) && (game_space[8] == gamepiece)) {
        isWinner = true;
        winner = gamepiece;
    }
    else if ((game_space[3] == gamepiece) && (game_space[6] == gamepiece) && (game_space[9] == gamepiece)) {
        isWinner = true;
        winner = gamepiece;
    }
    //Horizontal Win
    else if ((game_space[1] == gamepiece) && (game_space[2] == gamepiece) && (game_space[3] == gamepiece)) {
        isWinner = true;
        winner = gamepiece;
    }
    else if ((game_space[4] == gamepiece) && (game_space[5] == gamepiece) && (game_space[6] == gamepiece)) {
        isWinner = true;
        winner = gamepiece;
    }
    else if ((game_space[7] == gamepiece) && (game_space[8] == gamepiece) &&     (game_space[9] == gamepiece)) {
        isWinner = true;
        winner = gamepiece;
    }
    //Diagonal Win
    else if ((game_space[1] == gamepiece) && (game_space[5] == gamepiece) && (game_space[9] == gamepiece)) {
        isWinner = true;
        winner = gamepiece;
    }
    else if ((game_space[3] == gamepiece) && (game_space[5] == gamepiece) && (game_space[7] == gamepiece)) {
        isWinner = true;
        winner = gamepiece;
}
    else {
        //Determine if board is full
        if ((game_space[1] == ' ') || (game_space[2] == ' ') || (game_space[3] == ' ') || (game_space[4] == ' ') || (game_space[5] == ' ') || (game_space[6] == ' ')
        || (game_space[7] == ' ') || (game_space[8] == ' ') || (game_space[9]   == ' ')) {
            fullBoard = false;
        }
        else {fullBoard = true;}
        if (fullBoard == true) {
            isWinner = true;
            winner = 'T';
        }
    }
}
char Board::get_winner() {
    return(winner);
}
void Board::reset_winner() {
    winner = 'T';
}

Answer 1

Main review

• You are calling reset_winner() right after you created your Board. A preferred behaviour would be that it is 'well' initialized in the constructor (well, in your case, technically not in the ctor)

• You may consider passing the players name to the Board constructor (but that's only a possibility)

• You have a while (newgame == true), you may replace it by while (newgame) (same for the other while loops). This will make your code more readable.

You may aswell replace

        if (playAgain == "Y" || playAgain == "y") {
            newgame = true;
        } else if (playAgain == "N" || playAgain == "n") {
            newgame = false;
        } else {
            newgame = false;
        }

by a simple

        if (playAgain == "Y" || playAgain == "y") {
            newgame = true;
        } else {
            newgame = false;
        }

or even, as corvus_192 points out:

        newgame = playAgain == "Y" || playAgain == "y";

Also, people usually prefer return 0; instead of return (0);, even though that's (almost) just a matter of opinion

Board review

names

There are a few things I would rename, for instance:

• game_space isn't clear, I'd use game_board instead.
• According to its name, isWinner should be true if someone won, but in fact, it's true if the game is ended (in case of a victory OR in case of tie). game_over or something like that would be better. You may even use winner for that: 'X'/'O'if someone won, '\0' if no one won, and 'T' for a tie.
• imho, set_playerX/O should be set_playerX/O_name
• get_isWinner may be called is_game_over. As a rule of thumb, for a boolean x, its getter is usually called is_x

use for loops

You may use for loops to make your life easier and make your code more readable when determining if someone won. Assuming you use a 2d array (see the suggestions):

// Vertical
for (int i = 0 ; i < 3 ; ++i) {
    if (game_space[i][0] == game_space[i][1] && game_space[i][0] == game_space[i][2]) {
        winner = game_space[i][0];
        isWinner = true;
        return;
    }
}

// Vertical
for (int i = 0 ; i < 3 ; ++i) {
    if (game_space[0][i] == game_space[1][i] && game_space[0][i] == game_space[2][i]) {
        winner = game_space[0][i];
        isWinner = true;
        return;
    }
}

However you will still have to check 'by hand' for diagonals.

The same comment goes for checking for a full board:

// if there was no winner, we'll check for a tie
for (int i = 0 ; i < 3 ; ++i) {
    for (int j = 0 ; j < 3 ; ++j) {
        // if the board isn't full, we jump out of this method
        if (game_space[i][j] == ' ') {
            return;
        }
    }
}

// if there was no winner and the board is full (ie: this is a tie):
isWinner = true;
winner = 'T';

Suggestions

Use a 2d array

You may want to use a 2d std::array instead of a 1d raw array for game_space (ie: std::array<std::<array<char,3>,3> game_space, using it like a char[3][3]), as the tic-tac-toe has a 2d grid.

Use an enum

You might want to use an enum as player type. If you do it this way:

enum piece_color : char {
    empty = ' ',
    player_x = 'X',
    player_o = 'O',
    invalid = '\0' // for the 'winner' variable
};

you can even print it right away. Using an enum (or even better: an enum class) will enforce the type contained in the board, so that you are less likely to accidentaly put an invalid value.

Change selection_player

You may change the behavior of selection_playerX/O to make it this way:

• Take three parameters: the X position, Y position [if you chose to use a 2d grid], and the player value ('X' or 'O').
• Return a bool: true if it was possible to place the pawn, false if it wasn't
• Finally, rename it to something like play

Also change get_playerX/O into get_player_name and make it take the player as parameter

Why should you do this ? Because it allows you to change this:

    //Gameplay
    while (board.get_isWinner() == false) {
        int selection{0};

        //Player X's move.
        board.set_validSelection(false);
        while (board.get_validSelection() == false) {
            cout << board.get_playerX() << "'s move:";
            cin >> selection;
            board.selection_playerX(selection);
        }
        board.display();
        board.winOrTie('X');

        //Player O's move.
        if (board.get_isWinner() == false) {
            board.set_validSelection(false);
            while (board.get_validSelection() == false) {
                cout << board.get_playerO() << "'s move:";
                cin >> selection;
                board.selection_playerO(selection);
            }
            board.display();
            board.winOrTie('O');
        }
    }

    //Announce winner.
    if (board.get_winner() == 'X') {
        cout << board.get_playerX() << " is the winner!" << endl;
    } else if (board.get_winner() == 'O') {
        cout << board.get_playerO() << " is the winner!" << endl;
    } else if (board.get_winner() == 'T') {
        cout << "Game ends in a tie." << endl;
    }

into the much more clear:

    char player = 'X';
    int selection{0};
    while (!board.get_isWinner()) {
        do {
            cout << board.get_player_name(player) << "'s move: " << endl;
            cin >> selection;
                          //      X             Y
        } while (!board.play(selection % 3, selection / 3, player));
        // next player
        player = (player == 'X' ? 'O' : 'X');

    }

    if (board.is_tie()) {
         cout << "Game ends in a tie." << endl;
    } else {
         cout << board.get_player_name(board.get_winner()) << " is the winner!" << endl;
    }

Answer 2

Some things that stand out:

• Don't use using namespace std

It is bad practice as it leads to namespace pollution

• Testing boolean values

You do a lot of while (condition == true) or if (condition == false). You should prefer doing while (condition) and if (!condition) (note the "!")

Answer 3

Partial answer to Board.hpp: "Why wont std::vector game_space(10,' ') work here?"

You can (and should) use std::vector (or maybe std::array, as the dimension of the game_space is not expected to change). The correct C++11 initialization syntax would be either

std::vector<char> game_space {' ',' ',' ',' ',' ',' ',' ',' ',' ',' '};

or

std::vector<char> game_space=std::vector<char>(10, ' ');

In fact the method you have suggested in Board.hpp would work for e.g. declaring and simultaneously initializing a local variable - but it's not valid C++11 member init syntax.

Answer 4

1. Never use using namespace std;. Or do you know exactly which identifiers that introduces into your scope, on your current implementation, as well as in any older, newer, or unrelated one?
   See "Why is “using namespace std;” considered bad practice?" for more details, and amusing ways it can explode on you.

2. Avoid streaming multiple literals one after the other if at all possible. Merging them looks better and leads to more efficient code.
   String-literals which are only separated by whitespace are merged by the compiler even if they are on different lines.

3. Only flush a stream if you have to. Remember that doing so is quite costly, and that the standard streams are coupled. And if you really need it, using std::flush is more explicit.

4. There is no glory in verbosity. Unless being more verbose makes code adequately easier to understand, prefer brevity.

   • Desist from comparing a bool with a boolean literal. Just use it directly, or use the negtion operator !exp.
   • Refrain from using a boolean expression in an if-else-statement, just to assign a boolean value to the same variable in both paths. Assign it directly.
   • If you have to use/assign a different expression depending on some control-expression, look into the conditional operator exp ? true_exp : false_exp.

5. Prefer encoding program-flow with control-structures instead of using flags. Tracing variable-assignments is far more involved than scanning for dedicated control-flow-constructs.

6. One should be able to break off the game at nearly every place. Currently, trying to do so will just lead to re-prompting.

7. If there's an error with the input-stream, the new-game prompter will simply assume that you wanted a new game. That's suboptimal.

8. Others already answered your question about in-class-initializers. Still, I wanted to add that you can initialize a char-array with a string-literal, and don't have to make space for the implicit 0-terminator.

9. get_playerX() and get_playerO() should return a constant reference instead of a copy, so the caller can decide whether he needs a copy or not.

10. Your accessors are so simple, I would define them inline, if not make the underlying field public and remove them.

11. You are missing an in-class-initializer for winner. Or a user-defined ctor initializing it. Don't expect the user to call some init()-function, especially one curiously called reset_winner().

12. The Board currently scatters disjointed parts of its state in multiple members: isWinner, validSelection and winner. I suggest merging them into one enum:

    enum /* class ? */ State { turn_x, turn_o, win_x, win_o, draw, } state;
    

13. selection_playerX() and selection_playerO() can be merged. You don't even need additional arguments, as the overhauled board-state includes whose turn it is.

14. Test for a winner or tie at the end of placing a new marker. No need to wait.

15. It is curious that the include-guards in Board.hpp only surround the includes, instead of everything. That rather defeats the purpose.

16. I earnestly advise you to keep all your files fully lowercase. While you succeeded in staying consistent, there's no reason to make things any harder than neccessary by mixing cases.