Tic-Tac-Toe implementation c++

Question

I created this game just to practice my skills. I've been coding for a few years and I'm decent at data structures and algorithms. I want to know how to improve my implementation.

#include <iostream>
using namespace std;
const string QUIT = "q";
const string PLAY = "p";
class Player{
 // constructor
 public:
 Player(string player_symbol){
 this->player_symbol = "";
 }
 void play(string player_symbol);
 string get_player();
 void set_player(string symbol_str);
 private:
 int player_moves[10];
 string player_symbol;
};
string Player::get_player(){
 return player_symbol;
}
void Player::set_player(string symbol_str){
 this->player_symbol = symbol_str;
}
string board[10] = {"0","1","2", "3", "4", "5", "6","7","8","9"};
int victory_check(){
 // horizontal win
 if (board[1] == board[2] && board[2] == board[3]){
 if(board[1] == "X") return 1;
 else return 0;
 }
 if (board[4] == board[5] && board[5] == board[6]){
 if(board[4] == "X") return 1;
 else return 0;
 }
 if (board[7] == board[8] && board[8] == board[9]){
 if(board[8] == "X") return 1;
 else return 0;
 }
 // vertical
 if (board[1] == board[4] && board[4] == board[7]){
 if(board[7] == "X") return 1;
 else return 0;
 }
 if (board[2] == board[5] && board[5] == board[8]){
 if(board[5] == "X") return 1;
 else return 0;
 }
 if (board[3] == board[6] && board[6] == board[9]){
 if(board[6] == "X") return 1;
 else return 0;
 }
 // diagonal
 if (board[1] == board[5] && board[5] == board[9]){
 if(board[9] == "X") return 1;
 else return 0;
 }
 if (board[3] == board[5] && board[5] == board[7]){
 if(board[3] == "X") return 1;
 else return 0;
 }
 return -1;
}
void create_board();
void reset_board();
void check_illegal(int move_number, string player_symbol);
bool tie_game();
int main(){
 cout << "\n\n\tTic Tac Toe\n\n";
 cout << "Player 1 (X) - Player 2 (O)" << endl << endl;
 cout << endl;
 Player player_1("");
 player_1.set_player("X");
 Player player_2("");
 player_2.set_player("O");
 string player_move;
 bool game_over = false;
 bool player1_turn = true;
 bool tie = tie_game();
 tie = false;
 while(!game_over){
 create_board();
 if(player1_turn){
 cout << "PLAYER 1: enter your move: ";
 }
 else{
 cout << "PLAYER 2: enter your move: ";
 }
 cin >> player_move; 
 if(player_move == QUIT){
 game_over = true;
 }
 else if(player_move == PLAY){
 string my_symbol = player_1.get_player();
 if(player1_turn){
 player_1.play(my_symbol);
 }else{
 player_2.play(player_2.get_player());
 }
 }
 int winner = victory_check();
 if(winner == 1){
 create_board();
 cout << "CONGRATULATIONS Player 1, You won" << endl;
 cout << "Do you want to play again [y/n]" << endl;
 cin >> player_move;
 if(player_move == "y"){
 reset_board();
 }else{
 game_over = true;
 }
 }
 else if(winner == 0){
 create_board();
 cout << "CONGRATULATIONS Player 2, You won" << endl;
 cout << "Do you want to play again [y/n]" << endl;
 cin >> player_move;
 if(player_move == "y"){
 reset_board();
 create_board();
 }else{
 game_over = true;
 }
 game_over = true;
 }else if(tie == true){
 cout << "this game is a tie" << endl;
 }
 player1_turn = !player1_turn;
 }
 //player1_turn = false;
}
void create_board(){
 cout << " | | " << endl;
 cout << " " << board[1] << " | " << board[2] << " | " << board[3] << endl;
 cout << "_____|_____|_____" << endl;
 cout << " | | " << endl;
 cout << " " << board[4] << " | " << board[5] << " | " << board[6] << endl;
 cout << "_____|_____|_____" << endl;
 cout << " | | " << endl;
 cout << " " << board[7] << " | " << board[8] << " | " << board[9] << endl;
 cout << " | | " << endl << endl;
}
void reset_board(){
 for (int i = 0; i < 10; i++){
 board[i] = to_string(i);
 }
}
void Player::play(string player_symbol){
 int move_number;
 cout << "Enter your move number: ";
 cin >> move_number;
 while(board[move_number] == "X" || board[move_number] == "O"){
 cout << "This position is already taken, try again: ";
 cin >> move_number;
 }
 board[move_number] = player_symbol;
}
bool tie_game(){
 for(int i = 0; i < 10; i++){
 if (board[i] != "X" && board[i] != "O"){
 return false;
 }
 }
 return true;
}

• Mast: You have a player class, but the rest of the code is hardly object-oriented written. Is there a reason the other functions aren't methods?

Answer 1

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

Fix the bug

The code contains these two lines:

bool tie = tie_game();
tie = false;

I'm sure that you can see why that's a bug if you think about it for a bit.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Eliminate unused variables

The player_symbol parameter that is passed to the Player constructor is never used. Also player_moves isn't used anywhere. All of these should be eliminated from the code.

Avoid the use of global variables

I see that QUIT, PLAY and board are declared as global variables rather than as local variables. It's generally better to explicitly pass variables your function will need rather than using the vague implicit linkage of a global variable. For example, both QUIT and PLAY could easily be local to main and board could become an object with many of the functions turning into member functions.

Fix your formatting

There are inconsistent spaces at the beginning of lines, inconsistent indentation and inconsistent use and placement of curly braces {}. Being consistent helps others read and understand your code.

Don't use std::endl unless you really need to flush the stream

The difference between std::endl and '\n' is that std::endl actually flushes the stream. This can be a costly operation in terms of processing time, so it's best to get in the habit of only using it when flushing the stream is actually required. It's not for this code.

Use constant string concatenation

This code currently has a number of instances that look like this:

cout << "CONGRATULATIONS Player 2, You won" << endl;
cout << "Do you want to play again [y/n]" << endl;

This calls the << operator four times. Instead, you could write this:

cout << "CONGRATULATIONS Player 2, You won\n"
 "Do you want to play again [y/n]\n";

This only calls << once. The compiler automatically concatenates the string literals together.

Use all required #includes

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

Don't write getters and setters for every class

C++ isn't Java and writing getter and setter functions for every C++ class is not good style. Instead, move setter functionality into constructors and think very carefully about whether a getter is needed at all. In this code, there are better options for both getter and setter for Player, which emphasizes why they shouldn't be written in the first place.

Rethink the class design

The Player class holds only a single character, while the missing Board class has a lot more responsibility. I'd suggest removing the Player class and turning Board into an object. The design would be much clearer if you did so.