Making this Tic Tac Toe implementation more recursive

Question

Here is a Tic Tac Toe game in C++ that I have been working on. My questions are as follows:

1. I tested it for bugs, but if you find any, please post the solution!

2. I want to make the code more recursive, in a sense. Instead of having multiple lines of code that do the same things with different values, it would be nice to compress it even more. You see, I'm a minimalist and I'm lazy (fundamental programmer traits!) so less typing is good.

3. I'm wondering if my code follows the best common practices, and what those practices might be. For instance, is using camelCase for variables and something_like_this good for functions?

4. If you have any other comments on the code, please let me know!

main.cpp

/*Tic Tac Toe Game v1.0
Released under the GPL liscence version 2
Written by JohnBobSmith*/
#include <iostream>
#include <limits> //This is required to catch invalid user input
class tic_tac_toe //A class to contain all our functions
{
 public: //Allow us to use the functions anywhere
 char board[3][3]; //Creates an 2d array for the board
 char previousPlayerPiece; //variable used in multiple functions
 char previousPlayer2Piece; //varibable used in multiple functions
 void init_board()
 {
 //initializes a blank board
 for(int a = 0; a < 3; a++){
 std::cout << "\n";
 for (int b = 0; b < 3; b++){
 board[a][b] = '-';
 std::cout << board[a][b];
 }
 }
 }
 void print_board(char playerPiece, int pos1 = 0, int pos2 = 0)
 {
 //prints out the updated board when called upon to do so
 for(int a = 0; a < 3; a++){
 std::cout << "\n";
 for (int b = 0; b < 3; b++){
 board[pos1][pos2] = playerPiece;
 std::cout << board[a][b];
 }
 }
 }
 int check_for_overlap(int pos1, int pos2, char playerPiece)
 {
 switch(pos1)
 {
 case 1:
 if (pos2 == 1){
 if (board[0][0] != '-'){
 std::cout << "\nOVERLAP DETECTED!!!!!!" << std::endl;
 return true;
 }
 }
 if (pos2 == 2){
 if (board[0][1] != '-'){
 std::cout << "\nOVERLAP DETECTED!!!!!!" << std::endl;
 return true;
 }
 }
 if (pos2 == 3){
 if (board[0][2] != '-'){
 std::cout << "\nOVERLAP DETECTED!!!!!!" << std::endl;
 return true;
 }
 }
 case 2:
 if (pos2 == 1){
 if (board[1][0] != '-'){
 std::cout << "\nOVERLAP DETECTED!!!!!!" << std::endl;
 return true;
 }
 }
 if (pos2 == 2){
 if (board[1][1] != '-'){
 std::cout << "\nOVERLAP DETECTED!!!!!!" << std::endl;
 return true;
 }
 }
 if (pos2 == 2){
 if (board[1][2] != '-'){
 std::cout << "\nOVERLAP DETECTED!!!!!!" << std::endl;
 return true;
 }
 }
 case 3:
 if (pos2 == 1){
 if (board[2][0] != '-'){
 std::cout << "\nOVERLAP DETECTED!!!!!!" << std::endl;
 return true;
 }
 }
 if (pos2 == 2){
 if (board[2][1] != '-'){
 std::cout << "\nOVERLAP DETECTED!!!!!!" << std::endl;
 return true;
 }
 }
 if (pos2 == 3){
 if (board[2][2] != '-'){
 std::cout << "\nOVERLAP DETECTED!!!!!!" << std::endl;
 return true;
 }
 }
 }
 return false;
 }
 void update_board(char playerPiece)
 {
 //updates the board based on user input
 int x, y;
 std::cout << "Enter a position to place the " << playerPiece << " on the board" << std::endl;
 while (true){
 std::cout << "Enter row: " << std::endl;
 std::cin >> x;
 if (x < 1 || x > 3 || std::cin.fail()){
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(),'\n');
 std::cout << "Your number is invalid. Try again. " << std::endl;
 } else {
 break;
 }
 }
 while (true)
 {
 std::cout << "Enter col: " << std::endl;
 std::cin >> y;
 if (y < 1 || y > 3 || std::cin.fail()){
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(),'\n');
 std::cout << "Your number is invalid. Try again. " << std::endl;
 } else {
 break;
 }
 }
 if (check_for_overlap(x, y, previousPlayerPiece) == true){
 x--;y--;print_board(previousPlayer2Piece, x, y);
 std::cout << "\nThe board has been re-set. Try again!" << std::endl;
 } else if (check_for_overlap(x, y, previousPlayer2Piece) == true){
 x--;y--;print_board(previousPlayerPiece, x, y);
 std::cout << "\nThe board has been re-set. Try again." << std::endl;
 } else {
 x--;y--;print_board(playerPiece, x, y);
 }
 }
 /*
 This is probably the best way to write
 the update board function as it works,
 though it would be nice to have parts of
 it be more recursive...
 */
 int check_for_win(char playerPiece)
 {
 /*slices the board and checks if playerPiece occupies that slot.
 If the player makes a line, print that playerPiece has won
 and exit the game.*/
 if (board[0][0] == playerPiece && board[0][1] == playerPiece && board[0][2] == playerPiece){
 std::cout << "\nPlayer " << playerPiece << " wins!" << std::endl;
 return true;
 }
 else if(board[1][0] == playerPiece && board[1][1] == playerPiece && board[1][2] == playerPiece){
 std::cout << "\nPlayer " << playerPiece << " wins!" << std::endl;
 return true;
 }
 else if(board[2][0] == playerPiece && board[2][1] == playerPiece && board[2][2] == playerPiece){
 std::cout << "\nPlayer " << playerPiece << " wins!" << std::endl;
 return true;
 }
 else if(board[0][0] == playerPiece && board[1][0] == playerPiece && board[2][0] == playerPiece){
 std::cout << "\nPlayer " << playerPiece << " wins!" << std::endl;
 return true;
 }
 else if(board[0][1] == playerPiece && board[1][1] == playerPiece && board[2][1] == playerPiece){
 std::cout << "\nPlayer " << playerPiece << " wins!" << std::endl;
 return true;
 }
 else if(board[0][2] == playerPiece && board[1][2] == playerPiece && board[2][2] == playerPiece){
 std::cout << "\nPlayer " << playerPiece << " wins!" << std::endl;
 return true;
 }
 else if(board[0][0] == playerPiece && board[1][1] == playerPiece && board[2][2] == playerPiece){
 std::cout << "\nPlayer " << playerPiece << " wins!" << std::endl;
 return true;
 }
 else if(board[0][2] == playerPiece && board[1][1] == playerPiece && board[2][0] == playerPiece){
 std::cout << "\nPlayer " << playerPiece << " wins!" << std::endl;
 return true;
 } else {
 return false;
 }
 }
};
/*Wow, that was repetitive, just different
slices for the board...
*/
void run_game()
{
 char letter_o = 'O';
 char letter_x = 'X';
 tic_tac_toe ttt; //creates an object "ttt" to be used later
 ttt.previousPlayerPiece = letter_x;
 ttt.previousPlayer2Piece = letter_o;
 std::cout << "Welcome to tic tac toe!" << std::endl;
 std::cout << "Here is a blank board: " << std::endl;
 ttt.init_board();
 while (true){
 std::cout << "\nPlayer one, it is your turn\n" << std::endl;
 ttt.update_board(letter_x);
 if (ttt.check_for_win(letter_x) == true){
 break;
 }
 std::cout << "\nPlayer two, it is your turn\n" << std::endl;
 ttt.update_board(letter_o);
 if (ttt.check_for_win(letter_o) == true){
 break;
 }
 }
}
int main() //main kicks things off by calling "run_game()"
{
 run_game();
 return 0;
}

Answer 1

The most obvious improvement will come from the check_for_overlap function. You can replace your entire implementation with:

int check_for_overlap(int pos1, int pos2, char playerPiece)
{
 if (board[pos1-1][pos2-1] != '-'){
 std::cout << "\nOVERLAP DETECTED!!!!!!" << std::endl;
 return true;
 }
 return false;
}

The next thing I noticed was the difference between the user interface (1-based coordinates) and the internal representation (0-based coordinates). This is fine, but your implementation is a bit awkward in this area. You have lines like

 x--;y--;print_board(previousPlayer2Piece, x, y);

which is a pretty unusual style. An improvement would be:

 print_board(previousPlayer2Piece, x-1, y-1);

A better implementation might be to get the user input into different variables, such as userRow and userCol. So you would have (omitting your input validation and retry logic):

int userRow, userCol;
std::cin >> userRow;
std::cin >> userCol;
int x = userRow - 1;
int y = userCol - 1;

The == true check is redundant in a couple of places, since your functions already return bool. So instead of

 if (check_for_overlap(x, y, previousPlayerPiece) == true){

use

 if (check_for_overlap(x, y, previousPlayerPiece)){

In check_for_win, you can use loops to reduce the amount of code. For example:

 for (int i = 0; i < 3; i++) {
 if (board[i][0] == playerPiece && board[i][1] == playerPiece && board[i][2] == playerPiece){
 std::cout << "\nPlayer " << playerPiece << " wins!" << std::endl;
 return true;
 }
 }

You can use similar loops to check the column wins.

• JohnBobSmith: Okay, that sounds great! I was wondering if I could use a for loop to make the checking for win more recursive. Can I do the the same thing in the check_for_overlap function as well? Also, are my naming conventions good?
• Holloway: x--;y--;print_board(previousPlayer2Piece, x, y); and print_board(previousPlayer2Piece, x-1, y-1); will leave x and y in different states.

Answer 2

@Greg Hewgill has offered a better approach to doing this, but I'll mention some best-practice things about the code itself.

• The class name tic_tac_toe uses the same naming convention as your functions. It's usually preferred to have separate naming for types. User-defined types are typically named in PascalCase. As such, tic_tac_toe can be renamed as TicTacToe.

• This class is essentially just a struct since everything is public. Your three char variables would be considered data members, and they should be private. This will prevent them from being modified outside of the class directly. They're primarily supposed to be maintained within the class. In addition, if you have functions that shouldn't be accessible outside of the class, then they should be private as well.

• You should look into replacing that C-style array with a storage container, such as std::array or std::vector. They have many more benefits over C-style arrays, and they'll help keep your code cleaner and less error-prone.

• With so much code defined in the class, you should declare your data members and functions within the class and define them outside of it, perhaps in a separate file.

• You don't need to explicitly return 0 at the end of main(). The compiler will automatically do this for you as successful termination is implied.

• Robotnik: It's possible this is a university assignment, where using STL arrays/vectors is disallowed.
• JohnBobSmith: I'm not in university, im doing this as an enthusiast programmer :D.

Answer 3

Greg's answer covers the important stuff; I can contribute a few "best practices".

• (削除) std::cout << "\n"; is not the recommended way to emit a newline; you should use std::cout << std::endl; (削除ここまで) EDIT: see comment below
• I don't see any reason to make board, previousPlayerPiece nor previousPlayer2Piece public. Keep them private since they're only used inside the class.
• Some of the comments are not useful. The purpose of member variables is to be used by the methods.
• a and b are not informative variable names. Neither are pos1 and pos2 nor x and y which seem to be used for the same purpose. I suggest consistent row and column , perhaps with a prefix or suffix for what they're used for (e.g. playerMoveRow). If you really have to use a single-letter variable, i and j are typically used for generic loop indexes.
• Perhaps init_board() should just be the class constructor
• print_board() seems to have two functions: display the board, and update it with the player's latest move. I suggest splitting this into two functions
• board[pos1][pos2] = playerPiece; Why is this in the inner loop? That makes it get executed 9 times. It doesn't even belong in print_board().
• Change the while (true) loop into a do { } while loop. do is for loops that execute at least once.
• What is the purpose of the player_piece argument to check_for_overlap()?

• JohnBobSmith: Alright snowbody, I will implement all of your suggestions and post a pastebin of the updated code.
• Snowbody: I need to clarify something. You should use endl whenever you are done with your output, as it makes sure the output stream is flushed. It's not as necessary to use endl if you're going to send more output immediately (before anything else happens)