3
\$\begingroup\$

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;
}
\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot. I'll change whatever I can and post back. \$\endgroup\$ – Holyprogrammer Mar 6 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 Mar 6 at 15:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.