patterncppMinor
Tic Tac Toe algorithm
Viewed 0 times
tacalgorithmtictoe
Problem
I made a Tic Tac Toe game, but I think there's a lot of hard-coding and useless variables. Can you check it out and tell me what I can improve on? It works, but it has a couple of bugs.
#include
#include
#include
#include
#include
#include
using namespace std;
bool check_right_diagonal(char **arr, int ver_s, int hor_s, bool &win_p1, bool &win_p2, int i, bool &func_worked){
for (i = 0; i = i||k> p1_move_choice_i;
cin >> p1_move_choice_j;
if (check_occupied_pos(p1_move_choice_i, p1_move_choice_j, GameField, p1_mark, p2_mark)){
GameField[p1_move_choice_i][p1_move_choice_j] = NULL;
GameField[p1_move_choice_i][p1_move_choice_j] = 'X';
draw_field(GameField, ver_s);
p1_try = false;
p2_try = true;
}
else continue;
}
else if (p2_try){
cout > p2_move_choice_i;
cin >> p2_move_choice_j;
if (check_occupied_pos(p2_move_choice_i, p2_move_choice_j, GameField, p1_mark, p2_mark)){
GameField[p2_move_choice_i][p2_move_choice_j] = NULL;
GameField[p2_move_choice_i][p2_move_choice_j] = 'O';
draw_field(GameField, ver_s);
p2_try = false;
p1_try = true;
}
else continue;
}
WinGame(GameField, ver_s, hor_s, p1_mark, p2_mark, GAME_START);
}
}Solution
using namespace std;
This should be avoided. The reasons are explained in detail in that question and its answers on Stack Overflow. Instead you should add the namespace in front of every function call from that namespace, e. g.
Naming
You named two of your functions
Instead I recommend to just make one function
The convention would be to name it
Note that these names might still need to be changed if it turns out that we find a better way to do the algorithms.
Yet another capitalization style. This should be
You are using a lot of parameters and variables, so I won't list all of them, but basically all should have better speaking names, that tell you what they contain. In C++ names are often kept short, but for example your
Logic and Bugs
There seems to be a bug, or maybe just a redundancy, in the function
It seems like you want to check "has player 1 won but not player 2, or vice versa". Instead you check in the second part whether any of the two players has won, so the entire expression is equivalent to just
When checking whether a player has won, you do this:
I shortened the call to hide some of the parameters. You are passing the variable by reference and changing its value, which can easily lead to hard to find bugs, especially when there is an easier and more intuitive way of doing it. Furthermore you have different variables for both players whether they have won or not, and still the function is returning boolean whose value always seems to be -1.
If you changed your function to return the number of the player (1 or 2) if the player has won, and return 0 if none of them has won, you wouldn't need most of the parameters. Even better, if you only check whether the current player has won after he placed his symbol on the board, then you could just write the check in
(Note that again I omitted the parameters just for the exmaple.)
Doing it that way you can also avoid setting a variable named
Algorithms and Data Structures
You are checking the board as if it could have a dynamic size. But since you hardcoded the size to 3x3, and it is always 3x3 in tic-tac-toe, you can just do some simple checks instead of these 3 very long functions with nested loops.
This is the vertical check (board is your arr, since field fits better as a name):
This is the horizontal check:
```
if (board[0][0] ==
This should be avoided. The reasons are explained in detail in that question and its answers on Stack Overflow. Instead you should add the namespace in front of every function call from that namespace, e. g.
std::cout instead of cout. The additional typing is very little and the better readabilty (and avoiding further subtle problems) is worth it.Naming
You named two of your functions
check_right_diagonal and check_left_diagonal, however from reading the name it is still not obvious which diagonal is refered to, because how can a diagonal be on any side? You might differentiate the diagonals by calling them "upwards" and "downwards", but even that is only slightly better and still not precise.Instead I recommend to just make one function
check_diagonals, and check both in the same function. We will get to that later when we talk about the algorithms.check_verticalANDhorizontal_win is very unconventional, and unconventional is usually bad. It is important to be consistent with your naming scheme, otherwise it will be difficult to read the code.The convention would be to name it
check_vertical_and_horizontal_win. But now it is only consistent in style, still not logically. You didn't name the other functions check_diagonal_win, so I suggest to leave out the "win" here as well. After all it is pretty clear what is checked for in tic-tac-toe. Furthermore, a method should only do one thing, and method names that contain "and" are often a sign that it should be split up into two methods, here check_horizontal and check_vertical, or better and shorter check_rows and check_columns.Note that these names might still need to be changed if it turns out that we find a better way to do the algorithms.
TIC_TAC_TOE_PRINT is another atypical naming scheme, and the functions should rather be called something like print_tic_tac_toe_logo. The name you chose (aside from the capitalization) sounds more like a custom version of any print function, like for example printf.WinGameYet another capitalization style. This should be
win_game. Probably it should even be something like is_game_over or has_won, or something similar. But then it should be returning a boolean.You are using a lot of parameters and variables, so I won't list all of them, but basically all should have better speaking names, that tell you what they contain. In C++ names are often kept short, but for example your
arr is actually your board. The others are very cryptic, but we probably won't even need most of them. We will get to that.Logic and Bugs
There seems to be a bug, or maybe just a redundancy, in the function
WinGame.if (win_p1 && !win_p2 || win_p1 && win_p2)It seems like you want to check "has player 1 won but not player 2, or vice versa". Instead you check in the second part whether any of the two players has won, so the entire expression is equivalent to just
(win_p1 && win_p2), assuming that you don't have to explicitly check whether both players have won, which shouldn't be possible anyway.When checking whether a player has won, you do this:
check_verticalANDhorizontal_win(arr, ... , func_worked);
if (!func_worked)I shortened the call to hide some of the parameters. You are passing the variable by reference and changing its value, which can easily lead to hard to find bugs, especially when there is an easier and more intuitive way of doing it. Furthermore you have different variables for both players whether they have won or not, and still the function is returning boolean whose value always seems to be -1.
If you changed your function to return the number of the player (1 or 2) if the player has won, and return 0 if none of them has won, you wouldn't need most of the parameters. Even better, if you only check whether the current player has won after he placed his symbol on the board, then you could just write the check in
WinGame like this:if (is_vertical_win(...) || is_horizontal_win(...) || is_diagonal_win(...))(Note that again I omitted the parameters just for the exmaple.)
Doing it that way you can also avoid setting a variable named
func_worked, which is misleading because no matter whether the result is true or false, in both cases the function has worked. Just the result of the check would be different.Algorithms and Data Structures
You are checking the board as if it could have a dynamic size. But since you hardcoded the size to 3x3, and it is always 3x3 in tic-tac-toe, you can just do some simple checks instead of these 3 very long functions with nested loops.
This is the vertical check (board is your arr, since field fits better as a name):
if (board[0][0] == board[0][1] && board[0][0] == board[0][2]
|| board[1][0] == board[1][1] && board[1][0] == board[0][2]
|| board[2][0] == board[2][1] && board[2][0] == board[0][2])
{
return true;
}This is the horizontal check:
```
if (board[0][0] ==
Code Snippets
if (win_p1 && !win_p2 || win_p1 && win_p2)check_verticalANDhorizontal_win(arr, ... , func_worked);
if (!func_worked)if (is_vertical_win(...) || is_horizontal_win(...) || is_diagonal_win(...))if (board[0][0] == board[0][1] && board[0][0] == board[0][2]
|| board[1][0] == board[1][1] && board[1][0] == board[0][2]
|| board[2][0] == board[2][1] && board[2][0] == board[0][2])
{
return true;
}if (board[0][0] == board[1][0] && board[0][0] == board[2][0]
|| board[0][1] == board[1][1] && board[0][1] == board[2][1]
|| board[0][2] == board[1][2] && board[0][2] == board[2][2])
{
return true;
}Context
StackExchange Code Review Q#102076, answer score: 2
Revisions (0)
No revisions yet.