Tic Tac Toe - console application

Question

I have made a little Tic Tac Toe console application using OOP techniques and would love to get some advice and guidance on what I have done. I have tried to structure the program to be as organized as possible but I am sure there is a lot of room for improvement.

main.cpp

// TicTacToeOOPV2.cpp : Defines the entry point for the console application.
//
#include "stdafx.h"
#include "Board.h"
#include "Player.h"
#include <iostream>
#include <vector>
#include <string>
int main()
{
 Board board;
 Player player;
 std::vector<char> playingBoard(board.GetNumbOfSquares(), board.GetEmpty()); //The playing board
 bool gameOver = false;
 int move = 0, //Position player to on board
 player1Score = 0,
 player2Score = 0;
 char player1 = player.PlayerGamePiece(board);
 char player2 = player.OpponentGamePiece(board, player1); //Player two gets their gamePiece 
 //Decide first turn
 char turn = board.GetGamePieceX(); //Player who is 'X' gets the first turn
 board.DisplayBoard(playingBoard); //Display the pplaying board
 do
 {
 while (board.FindWinner(playingBoard) == board.GameOnGoing())
 {
 //If its player1's turn
 if (turn == player1)
 {
 std::cout << "\nPlayer 1: ";
 move = player.PlayerMove(board, playingBoard, move); //Get players move position
 playingBoard[move] = player1; //Assign positon onto playingboard
 turn = player2; //Change turn 
 }
 //If its player 2's turn
 else
 {
 std::cout << "\nPlayer 2: ";
 move = player.PlayerMove(board, playingBoard, move); //Get player2's move position
 playingBoard[move] = player2; //Assign move positionto playing board
 turn = player1; //Change to player 1's turn
 }
 board.DisplayBoard(playingBoard); //Display the playing board to players 
 }
 board.AnnounceRoundWinner(board.FindWinner(playingBoard), player1, player2, player1Score, player2Score);
 board.DisplayScores(player1Score, player2Score);
 board.ClearPlayingBoard(playingBoard);
 } while (!player.CheckGameOver(gameOver, player1Score, player2Score));
 //Announce winner
 board.AnnounceWinner(player, player1Score, player2Score);
 //board.AnnounceWinner(board.FindWinner(playingBoard), player1, player2); //Announce the winner of the game
 //Keep window open
 std::string barn;
 std::cin >> barn;
 return 0;
}

Player.h

#pragma once
#include <vector>
class Board;
class Player
{
private:
 const int winningScore = 1; //Score that player must reach to win game
public:
 int GetWinningScore();
 bool DecideFirstTurn(Board& board); //Decides who gets the first turn
 char PlayerGamePiece(Board&board); //Player gets their game piece
 char OpponentGamePiece(Board& board, char player1); //Opponent gets their gamne piece
 int PlayerMove(Board& board, const std::vector<char>& playingBoard, int move); //Gets the players movement on board
 int PositionOfMove(const std::vector<char>& playingBoard, int high, int low); //Asks player where they would like to move on board
 bool CheckGameOver(bool gameOver, int player1Score, int player2Score); //Check to see if scores have reached maximum amount
};

Player.cpp

#include "stdafx.h"
#include "Player.h"
#include "Board.h"
#include <iostream>
int Player::GetWinningScore()
{
 return winningScore;
}
//Decides what player gets the first turn
bool Player::DecideFirstTurn(Board& board)
{
 char responce;
 std::cout << "Do you wish to go first? 'y' - Yes. 'n' - No. ";
 std::cin >> responce;
 switch (responce)
 {
 case 'y':
 return true;
 break;
 case 'n':
 return false;
 break;
 default:
 std::cout << "Did not enter the right data.";
 }
}
//Returns apprioriate game piece to who gets the first turn
char Player::PlayerGamePiece(Board& board)
{
 bool firstTurn = DecideFirstTurn(board);
 if (!firstTurn)
 {
 return board.GetGamePieceO();
 }
 else
 {
 return board.GetGamePieceX();
 }
}
//Opponent gets opposite game piece than player
char Player::OpponentGamePiece(Board& board, char player1)
{
 if (player1 == board.GetGamePieceX())
 {
 return board.GetGamePieceO();
 }
 else
 {
 return board.GetGamePieceX();
 }
}
int Player::PlayerMove(Board& board, const std::vector<char>& playingBoard, int move)
{
 bool moveAllowed = false;
 do
 {
 move = PositionOfMove(playingBoard, playingBoard.size() - 1, 0);
 if (board.MoveIsLegal(playingBoard, move))
 {
 moveAllowed = true;
 }
 else
 {
 std::cout << "\nMove not allowed." << "\n";
 }
 } while (!moveAllowed);
 return move; 
}
int Player::PositionOfMove(const std::vector<char>& playingBoard, int high, int low)
{
 int numb = 0;
 do
 {
 std::cout << "\n\nEnter a digit between " << low << " and " << high << "\n";
 std::cin >> numb;
 } while (numb > high || numb < low);
 return numb;
}
bool Player::CheckGameOver(bool gameOver, int player1Score, int player2Score)
{
 if (player1Score < winningScore && player2Score < winningScore)
 return false;
 return true;
}

Board.h

#pragma once
#include <vector>
class Player;
class Board
{
private:
 const char X = 'X';
 const char O = 'O';
 const char TIE = 'T';
 const char NOONE = 'N';
 const char EMPTY = ' ';
 const int numbOfSquares = 9; //Number of squares on playingBoard
public:
 char GetGamePieceX(); 
 char GetGamePieceO();
 int GetNumbOfSquares(); //Gets number of squares on playing board
 char GetEmpty();
 char GameOnGoing(); 
 void DisplayBoard(const std::vector<char>& playingBoard); //Display the board
 char FindWinner(const std::vector<char>& playingBoard); //Find winning combinations, resulting in winer of game
 char IsGameTie(const std::vector<char>& playingBoard); //Is the game a tie?
 bool MoveIsLegal(const std::vector<char>& playingBoard, int move); //Determine whether the requested move is legal
 void AnnounceWinner(Player& player, int player1Score, int player2Score); //Announce the winner of the game
 void AnnounceRoundWinner(char winner, char player1, char player2, int& player1Score, int& player2Score); //Announces the winner of the round
 void ClearPlayingBoard(std::vector<char>& playingBoard); //Clear the playing board
 void DisplayScores(int player1Score, int player2Score);
};

Board.cpp

#include "stdafx.h"
#include "Board.h"
#include "Player.h"
#include <iostream>
//Get game piece 'X'
char Board::GetGamePieceX()
{
 return X;
}
//Get game piece X
char Board::GetGamePieceO()
{
 return O;
}
//Get number of squares that are on the board
int Board::GetNumbOfSquares()
{
 return numbOfSquares;
}
//Get the empty square on the playing board
char Board::GetEmpty()
{
 return EMPTY;
}
//Nobody has won, game still on-going
char Board::GameOnGoing()
{
 return NOONE;
}
//Display the board to players
void Board::DisplayBoard(const std::vector<char>& playingBoard)
{
 std::cout << "\n\t" << playingBoard[0] << " | " << playingBoard[1] << " | " << playingBoard[2];
 std::cout << "\n\t" << "---------";
 std::cout << "\n\t" << playingBoard[3] << " | " << playingBoard[4] << " | " << playingBoard[5];
 std::cout << "\n\t" << "---------";
 std::cout << "\n\t" << playingBoard[6] << " | " << playingBoard[7] << " | " << playingBoard[8];
}
char Board::FindWinner(const std::vector<char>& playingBoard)
{
 /*
 The playing board
 0, 1, 2
 3, 4, 5
 6, 7, 8
 */
 //All possible winning combinations 
 const int winningRows[8][3] =
 {
 //Horizontal combinations
 { 0, 1, 2 },
 { 3, 4, 5 },
 { 6, 7, 8 },
 //Vertical combinations
 { 0, 3, 6 },
 { 1, 4, 7 },
 { 2, 5, 8 },
 //Diagonal combinations
 { 0, 4, 8 },
 { 2, 4, 6 }
 };
 const int maxRows = 8; //Maximum amount of rows to search through to find a winning combination
 //const int winningRows = WinningRows();
 //Search for winning combination
 for (int row = 0; row < maxRows; row++)
 {
 if ((playingBoard[winningRows[row][0]] != EMPTY) &&
 (playingBoard[winningRows[row][0]] == playingBoard[winningRows[row][1]]) &&
 (playingBoard[winningRows[row][1]] == playingBoard[winningRows[row][2]]))
 {
 //Return fisrt character of winning combination for winner
 return playingBoard[winningRows[row][0]];
 }
 }
 //If a winning combination wasn't found,
 //Declare if game is TIE or game is still on-going
 return IsGameTie(playingBoard);
}
char Board::IsGameTie(const std::vector<char>& playingBoard)
{
 //If there are no more empty squares on game board, game is tie
 if (count(playingBoard.begin(), playingBoard.end(), EMPTY) == 0)
 {
 return TIE;
 }
 //If there are still empty squares, game is still on-going
 else
 {
 return NOONE;
 }
}
//Is square where player wants to move empty
bool Board::MoveIsLegal(const std::vector<char>& playingBoard, int move)
{
 return (playingBoard[move] == EMPTY);
}
void Board::AnnounceWinner(Player& player, int player1Score, int player2Score)
{
 if (player1Score >= player.GetWinningScore())
 std::cout << "\n\nPlayer 1 has won the game!" << "\n";
 if (player2Score >= player.GetWinningScore())
 std::cout << "\n\nPlayer 2 has won." << "\n";
}
void Board::AnnounceRoundWinner(char winner, char player1, char player2, int& player1Score, int& player2Score)
{
 //If player1 has won the round.
 if (winner == player1)
 {
 std::cout << "\nPlayer 1 has won the round.";
 player1Score++;
 }
 //If player 2 has won the round
 else if (winner == player2)
 {
 std::cout << "\nPlayer 2 has won the round.";
 player2Score++;
 }
 else
 {
 std::cout << "\nGame is tie.";
 }
}
void Board::ClearPlayingBoard(std::vector<char>& playingBoard)
{
 std::cout << "\n\n\n";
 std::vector<char> board(numbOfSquares, EMPTY);
 playingBoard.swap(board);
}
void Board::DisplayScores(int player1Score, int player2Score)
{
 std::cout << "\nRound over!\n";
 std::cout << "Player1's score: " << player1Score << ", " << "Player2's score: " << player2Score;
}

Answer 1

First things first: your code is pretty easy to read, and very consistently formatted, good job on that.

You're including the right headers, and didn't go wild with using directives, and that's good too. (Note that stdafx.h is a Microsoft-specific thing for precompiled headers I think. It's not portable, and I doubt you need it here. Consider removing it.)

Now on the design of your solution: there's something very strange. You created two classes, Player and Board. With such names, I was expecting them to actually be participants in the game. But looking closer, I see you only ever instantiate one player. That looks real odd for a game that requires two. So, closer look at the classes and here's what jumps out: they are just collections of functions.

Neither class has any real state - all their members are const and might as well be static. (Player's only member doesn't really belong there either - how many games are required to win is a property of the game (or the umpire/referee maybe), but not of the player.)

So where did all the game state go? Well, it's all in your main function. It has the tedious job of maintaining all the game's state by itself, calling into helper functions from Player or Board passing in all the various required bits and pieces of state each time.

That's not a very good design. main does too much bookkeeping, the two classes don't do enough. Your game is going to be hard to extend: try to think of how much you need to change your code if you decide to implement an AI player, and let the user play against it. That's going to be very hard to do with your current code. But it should "only" require creating a new player class with the logic, and a few initialization changes.

---

Here's a proposal about how to model the game (there are many ways to do it, this is just a rough idea):

• A player has a symbol (X or O) and can pick a move on a board.
• A board knows what moves have been played, can check and record a move, and check the game status (undecided, won, tie).
• The referee/game controller keeps a set of players, tracks their score, owns the board, and runs the game: asks each player in turn to pick a move, gets it recorded if valid, checks for stalemate, etc.

The words in italics are supposed to be hints about what properties/methods the classes should have.

The game controller's "round loop" would look this like:

int position = players[current]->pickMove(board);
if (!board.isValidMove(position)) {
 // give players[current] a red card, escort out of the field
} else {
 board.set(position, players[current]->symbol());
}
if (board.isTied()) {
 // announce a tie, move to next round (i.e. return)
} else if (board.isWon()) {
 // announce current player as a winner
 // tally score
 // move to next round
} else { /* change players */ }

The game controller's "game loop" would:

• check if either player has won
• if not, play a round
  • back to step one.
• if so, congratulate player and return.

With that in place, you'll be able to extend your game with AI players for instance. (With a careful design of a Player base class with a virtual pickMove function for instance, and HumanPlayer and Bot subclasses.)

---

Player::DecideFirstTurn is buggy. If the user enters a wrong value, the code flows out of the function without returning. Since it is not a void function, this leads to undefined behavior.

• Ryan Swann: Hey, thank you for your comment! I have taken the feedback on board and have made another program that incorporates what you have said. codereview.stackexchange.com/questions/115448/…

Answer 2

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

Isolate platform-specific code

If you must have stdafx.h, consider wrapping it so that the code is portable:

#ifdef WINDOWS
#include "stdafx.h"
#endif

Make sure all paths return a value

The Player::DecideFirstTurn routine is declared as returning bool but only actually returns a value when the player responds either 'y' or 'n'. What if it's some other character? Either return an explicit value of some kind (perhaps false?) or throw an exception. Alternatively, stay in the function until the user gives a recognized answer.

Eliminate unused variables

The parameter playingBoard in the Player::PositionOfMove() function, the gameOver parameter in Player::CheckGameOver() and the board parameter in Player::DecideFirstTurn(), are all unused variables and should be omitted from the program.

Make sure to #include all required headers

This program refers to std::count in Board.cpp but does not include the corresponding header. Fix that by adding this line:

#include <algorithm>

Specify the appropriate namespace

The code in Board.cpp currently refers to count but should actually use the correct namespace which is std::count as in:

if (std::count(playingBoard.begin(), playingBoard.end(), EMPTY) == 0)

Use const where practical

The current Board::DisplayBoard() routine does not (and should not) modify the underlying object, and so it should be declared const:

void Board::DisplayBoard(const std::vector<char>& playingBoard) const;

Simplify expressions

The code currently includes the following code (once the previous few points are addressed):

char Board::IsGameTie(const std::vector<char>& playingBoard) const
{
 //If there are no more empty squares on game board, game is tie
 if (std::count(playingBoard.begin(), playingBoard.end(), EMPTY) == 0)
 {
 return TIE;
 }
 //If there are still empty squares, game is still on-going
 else
 {
 return NOONE;
 }
}

First, in C++, any non-zero integer value is true so there isn't any need to explicitly compare to 0. Second, you can use a single return statement here with the ternary operator.

//Is square where player wants to move empty
bool Board::MoveIsLegal(const std::vector<char>& playingBoard, int move)
{
 return (playingBoard[move] == EMPTY);
}

Eliminate useless functions

The Board::GetGamePieceX() and Board::GetGamePieceO() functions are not really useful. Instead, since the underlying data item is already const and not changeable within the code, you could instead simply make Board::X and Board::O public data members and use them directly. The same is true for all of the accessor functions for TIE, NOONE, EMPTY and numbOfSquares as well and Player::winningScore.

Name bool functions for the question they answer

The Board::IsGameTie() function is well named. When it returns true, it's easy to understand from context that this means that the game is a tie. In that same spirit, your Board::MoveIsLegal would be better named as Board::IsMoveLegal.

Simplify member function names

With DisplayBoard() being a member of the Board object, it seems redundant to have to type board.DisplayBoard(). I think that Board::Display() would be a better name.

Better encapsulate data within objects

It seems to me that playingBoard should be private member data of the Board object. Not only would it considerably simplify the interface, it would also enforce encapsulation so that the only way to modify a Board object is through the provided interfaces.

Make member functions static where appropriate

None of the Player member functions actually refer to any private member data, because there is none. This strongly suggests that either one should make all of these functions static, that they should be within a namespace rather than a class or that perhaps there is data that should be associated with a player that is missing. In this case, the Player::DecideFirstTurn() function neither reads nor writes any Player data, so it should probably be made static. However, it also seems clear that player scores are obvious pieces of data that might be made private member data, so I'd advise moving the scores to within the Player class and then creating two Player objects -- one for X and one for O.

Consider further encapsulation

It seems to me that the Board and the two Player objects postulated above might all reasonably be part of an overall Game class that would manage all of the things that are currently part of main(). That would also be the logical location for such member functions as AnnounceRoundWinner() and DisplayScores which are somewhat awkward as Board object member functions.

Think of the user

It's not intuitively obvious how to enter a move when the game first starts. It would be better to consider the user and to offer a prompt (at least at the beginning) showing how the squares are numbered.

Eliminate return 0 at the end of main

When a C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no reason to put return 0; explicitly at the end of main.

• Ryan Swann: Hey, thank you for your comment! I have taken the feedback on board and have made another program that incorporates what you have said. codereview.stackexchange.com/questions/115448/…

Answer 3

Ternary

The ternary expression is a less powerful and shorter version of the if statement. While the if statement allows statements or expressions, the ternary allows only expressions. It simplifies and shortens the code quite a lot in some cases:

char Player::PlayerGamePiece(Board& board)
{
bool firstTurn = DecideFirstTurn(board);
if (!firstTurn)
{
 return board.GetGamePieceO();
}
else
{
 return board.GetGamePieceX();
}
}

Becomes:

char Player::PlayerGamePiece(Board& board)
{
 return DecideFirstTurn(board) ? board.GetGamePieceX() : board.GetGamePieceO();
}

The second version is functionally identical to the first.

Also other functions would benefit from a ternary, for example Board::IsGameTie. To avoid overly long lines, I suggest creating a bool helper variable containing the condition when it is complicated.