I am really new to programming, and I made this simple Tic Tac Toe app in C++. Please tell me what can be improved.
#include <iostream>
#include <vector>
#include <algorithm>
using namespace std;
vector<string> toDraw{"1", "2", "3", "4", "5", "6", "7", "8", "9"}; // NOLINT(cert-err58-cpp)
void reset();
void reDrawBoard(bool gameOver);
int playerTurn = 1;
int check(); // 0 - Game in progress; 1 - Player 1 wins; 2 - Player 2 wins;
int turnsPlayed = 0;
int main() {
while (true) {
reDrawBoard(false);
string input;
int inputToInt;
while (true) {
input = "";
cin >> input;
try {
inputToInt = stoi(input);
if (inputToInt > 9) {
cout << "\nInvalid Value. Try again.";
cout << "\nPlayer " + to_string(playerTurn) + ", Enter a number:";
continue;
}
}
catch (exception &e) {
cout << "\nInvalid Value. Try again.";
cout << "\nPlayer " + to_string(playerTurn) + ", Enter a number:";
continue;
}
try {
if (inputToInt == stoi(toDraw.at(static_cast<unsigned long>(inputToInt - 1)))) {
break;
}
}
catch (exception &e) {
cout << "Position already occupied. Please try again.";
cout << "\nPlayer " + to_string(playerTurn) + ", Enter a number:";
continue;
}
}
if (playerTurn == 1) {
toDraw[inputToInt - 1] = "X";
turnsPlayed++;
playerTurn = 2;
} else {
toDraw[inputToInt - 1] = "O";
turnsPlayed++;
playerTurn = 1;
}
int result = check();
if (result == 1) {
reDrawBoard(true);
cout << "PLAYER ONE WINS!!\n";
cout << "Would you like to play again? (Y/N): ";
string response;
cin >> response;
transform(response.begin(), response.end(), response.begin(), ::tolower);
if (response != "y") {
printf("\e[1;1H\e[2J");
return 0;
}
reset();
continue;
}
if (result == 2) {
reDrawBoard(true);
cout << "PLAYER TWO WINS!!\n";
cout << "Would you like to play again? (Y/N): ";
string response;
cin >> response;
transform(response.begin(), response.end(), response.begin(), ::tolower);
if (response != "y") {
printf("\e[1;1H\e[2J");
return 0;
}
reset();
continue;
}
if (turnsPlayed == 9) {
cout << "DRAW!!\n";
cout << "Would you like to play again? (Y/N): ";
string response;
cin >> response;
transform(response.begin(), response.end(), response.begin(), ::tolower);
if (response != "y") {
printf("\e[1;1H\e[2J");
return 0;
}
reset();
}
}
}
void reDrawBoard(bool gameOver) {
printf("\e[1;1H\e[2J");
cout <<
"TIC TAC TOE - Made by Khushraj (a.k.a Holyprogrammer)\nPlayer one in X and Player two is O\n\n | | \n " +
toDraw.at(0) +
" | " + toDraw.at(1) + " | " +
toDraw.at(2) + " \n___|___|___\n | | \n " + toDraw.at(3) + " | " + toDraw.at(4) + " | " +
toDraw.at(5) +
" \n___|___|___\n | | \n " + toDraw.at(6) + " | " + toDraw.at(7) + " | " + toDraw.at(8) +
" \n | | \n\n";
if (!gameOver) {
cout << "Player " + to_string(playerTurn) + ", Enter a number: ";
}
/*
* This prints (something similar to) -
*
* TIC TAC TOE - Made by Khushraj
* Player one is X and Player two is O
*
* | |
* 1 | 2 | 3
* ___|___|___
* | |
* 4 | 5 | 6
* ___|___|___
* | |
* 7 | 8 | 9
* | |
*
* Player <1 || 2>, Enter a number:
*/
}
int check() { // 0 - Game in progress, 1 - Player one wins, 2 - Player two wins
string one = toDraw[0];
string two = toDraw[1];
string three = toDraw[2];
string four = toDraw[3];
string five = toDraw[4];
string six = toDraw[5];
string seven = toDraw[6];
string eight = toDraw[7];
string nine = toDraw[8];
//FOR PLAYER ONE
// If the player has 3X in a row, then
if ((one == "X" && two == "X" && three == "X") || (one == "X" && four == "X" && seven == "X") ||
(one == "X" && five == "X" && nine == "X") || (seven == "X" && five == "X" && three == "X") ||
(seven == "X" && eight == "X" && nine == "X") || (three == "X" && six == "X" && nine == "X") ||
(four == "X" && five == "X" && six == "X") || (two == "X" && five == "X" && six == "X")) {
return 1;
}
//FOR PLAYER TWO
// If the player has 3Y in a row, then
if ((one == "O" && two == "O" && three == "O") || (one == "O" && four == "O" && seven == "O") ||
(one == "O" && five == "O" && nine == "O") || (seven == "O" && five == "O" && three == "O") ||
(seven == "O" && eight == "O" && nine == "O") || (three == "O" && six == "O" && nine == "O") ||
(four == "O" && five == "O" && six == "O") || (two == "O" && five == "O" && eight == "O")) {
return 2;
}
return 0;
}
void reset() {
turnsPlayed = 0;
toDraw = {"1", "2", "3", "4", "5", "6", "7", "8", "9"};
playerTurn = 1;
}
1 Answer 1
Welcome to programming! Here are some things that may help you improve your program.
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. Know when to use it and when not to (as when writing include headers). In this particular case, I happen to think it's perfectly appropriate because it's a single short program and not a header. Some people seem to think it should never be used under any circumstance, but my view is that it can be used as long as it is done responsibly and with full knowledge of the consequences.
Avoid the use of global variables
I see that toDraw
, playerTurn
and turnsPlayed
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. See the next suggestion.
Use object orientation
Because you're writing in C++, it would make sense to collect things into a class such as TicTacToe
that could hold the toDraw
, playerTurn
and turnsPlayed
variables, and have reset
and reDrawBard
be member functions rather than separate functions. You may not yet have learned about objects or classes, but they're one of the main strengths of C++ and something you should learn soon if you haven't already. Use objects where they make sense.
Use appropriate data types
The code currently contains this code:
int check() { // 0 - Game in progress, 1 - Player one wins, 2 - Player two wins
string one = toDraw[0];
string two = toDraw[1];
string three = toDraw[2];
string four = toDraw[3];
string five = toDraw[4];
string six = toDraw[5];
string seven = toDraw[6];
string eight = toDraw[7];
string nine = toDraw[8];
//FOR PLAYER ONE
// If the player has 3X in a row, then
if ((one == "X" && two == "X" && three == "X") || (one == "X" && four == "X" && seven == "X") ||
(one == "X" && five == "X" && nine == "X") || (seven == "X" && five == "X" && three == "X") ||
(seven == "X" && eight == "X" && nine == "X") || (three == "X" && six == "X" && nine == "X") ||
(four == "X" && five == "X" && six == "X") || (two == "X" && five == "X" && six == "X")) {
return 1;
}
First, the comment is good, but the whole function could be improved if it instead returned an enum
or even better, an enum class
:
enum class GameState { inProgress, Player1Wins, Player2Wins, Tie };
Now we can use names instead of values and we are also assured that the return value must be one of these instead of any possible int
value.
Second, creating and naming all of those strings every time is both potentially slow and prone to error. In fact ...
Fix the bug
Part of the current logic for checking for a win as shown above is this:
|| (two == "X" && five == "X" && six == "X")) {
That should be squares two
, five
and eight
, not six
. I'd recommend rewriting the check
function entirely to avoid this.
Think carefully about the problem
By definition, only the player who just played can win, so instead of checking for both X
and O
wins each time, we can pass in a variable which indicates which player just played and only check that.
Make sure to #include
all required headers
This program calls printf
but does not include the corresponding header. Fix that by adding this line:
#include <cstdio>
Or better yet...
Don't mix printf
and iostream
There's little reason to require both <cstdio>
and <iostream>
in this program. Everywhere there is a printf
could instead be a std::cout <<
instead.
Don't use non-standard escape sequences
The \e
escape sequence, while common, is not a standard escape sequence. Instead, you could use \x27
or 033円
.
Don't Repeat Yourself (DRY)
There is a lot of repeated code here that only differs by which player token is being considered. Instead of repeating code, it's generally better to make common code into a function.
Eliminate magic numbers
The constants 3, 9 and the string "\e[1;1H\e[2J"
are used in multiple places. It would be better to have them as named const
values so that it would be clear what those numbers and string represent.
Declare the loop exit condition at the top
The while
loop inside main
currently says this:
while (true) {
but the loop doesn't really continue forever -- it exits when the player decides to quit the game. For that reason, I'd suggest instead that it be something like this:
bool playing = true;
while (playing) {
Then just set the condition within the loop at the appropriate place.
-
\$\begingroup\$ Thanks a lot. I'll change whatever I can and post back. \$\endgroup\$user191336– user1913362019年03月06日 15:21:52 +00:00Commented Mar 6, 2019 at 15:21
-
\$\begingroup\$ I'm glad it was useful to you. Just remember not the change the code in this question but ask a new one. See What to do when someone answers. \$\endgroup\$Edward– Edward2019年03月06日 15:55:16 +00:00Commented Mar 6, 2019 at 15:55