1
\$\begingroup\$

I created a program on Code::Blocks that allows you to play Tic-Tac-Toe with 2 players on the same program:

#include <iostream>
#include <cstdlib>
#include <cstdio>

using namespace std;

char square[10] = {'o','1','2','3','4','5','6','7','8','9'};

int checkwin();
void board();

int main()
{
    int player = 1, i, choice;

    char mark;
    do {
        board();
        player = (player % 2) ? 1 : 2;

        cout << "Player " << player << ", enter a number: ";
        cin >> choice;

        mark=(player == 1) ? 'X' : 'O';
        if (choice == 1 && square[1] == '1') {
            square[1] = mark;
        } else if (choice == 2 && square[2] == '2') {
            square[2] = mark;
        } else if (choice == 3 && square[3] == '3') {
            square[3] = mark;
        } else if (choice == 4 && square[4] == '4') {
            square[4] = mark;
        } else if (choice == 5 && square[5] == '5') {
            square[5] = mark;
        } else if (choice == 6 && square[6] == '6') {
            square[6] = mark;
        } else if (choice == 7 && square[7] == '7') {
            square[7] = mark;
        } else if (choice == 8 && square[8] == '8') {
            square[8] = mark;
        } else if (choice == 9 && square[9] == '9') {
            square[9] = mark;
        } else {
            cout << "Invalid move ";
            player--;
            cin.ignore();
            cin.get();
        }
        i = checkwin();
        player++;
    } while(i == -1);
    board();
    if (i == 1) {
        cout << "Player " <<--player<< " win ";
    } else {
        cout << "Game draw";
    }
    cin.ignore();
    cin.get();

    return 0;
}

int checkwin()
{
    if (square[1] == square[2] && square[2] == square[3]) {
        return 1;
    } else if (square[4] == square[5] && square[5] == square[6]) {
        return 1;
    } else if (square[7] == square[8] && square[8] == square[9]) {
        return 1;
    } else if (square[1] == square[4] && square[4] == square[7]) {
        return 1;
    } else if (square[2] == square[5] && square[5] == square[8]) {
        return 1;
    } else if (square[3] == square[6] && square[6] == square[9]) {
        return 1;
    } else if (square[1] == square[5] && square[5] == square[9]) {
        return 1;
    } else if (square[3] == square[5] && square[5] == square[7]) {
        return 1;
    } else if (square[1] != '1' && square[2] != '2' && square[3] != '3'&& square[4] != '4' && square[5] != '5' && square[6] != '6'&& square[7] != '7' && square[8] != '8' && square[9] != '9') {
        return 0;
    } else {
        return -1;
    }
}

void board()
{
    system("cls");
    cout << "\tTic Tac Toe";

    cout << "Player 1 (X)  -  Player 2 (O)" << endl << endl;
    cout << endl;

    cout << "     |     |     " << endl;
    cout << "  " << square[1] << "  |  " << square[2] << "  |  " << square[3] << endl;

    cout << "_____|_____|_____" << endl;
    cout << "     |     |     " << endl;

    cout << "  " << square[4] << "  |  " << square[5] << "  |  " << square[6] << endl;

    cout << "_____|_____|_____" << endl;
    cout << "     |     |     " << endl;

    cout << "  " << square[7] << "  |  " << square[8] << "  |  " << square[9] << endl;

    cout << "     |     |     " << endl << endl;
}

I realize from past posts that using namespace std is considered bad practice, though I used it because adding all the std::(anything) would be very tedious. I also realized that there is probably an if statement overflow, so I'm wondering what can help.

\$\endgroup\$
3
\$\begingroup\$

Counting starts at 0

Get used to the fact that in C and C++, counting starts at 0. You have a lot that starts at one, and leaves element 0 of arrays unused. That can be an acceptable strategy for handling things like porting existing Fortran code to C or C++, but it's Not how you want to write new code.

Meaningful Names

Rather than (for example) having checkwin return -1, 0, or 1 to indicate current status, I'd prefer to define an enumeration and return an instance of it:

enum { WON, DRAW, ONGOING };

At least to me, this makes client code like:

    i = checkwin();
    player++;
} while(i == ONGOING);
board();
if (i == WIN) {
    cout << "Player " <<--player<< " win ";
} else {
    cout << "Game draw";
}

... a lot more readable.

Verbosity

A lot of this strikes me as excessively verbose for what it accomplishes. Perhaps the most striking example would be your checkwin. Most of the code there can be reduced to a couple of simple loops with an array of indices into the main array:

int checkwin() {
    static const char index[][3] = {
        { 0, 1, 2 },
        { 3, 4, 5 },
        { 6, 7, 8 },
        { 0, 3, 6 },
        { 1, 4, 7 },
        { 2, 5, 8 },
        { 0, 4, 8 },
        { 2, 4, 6 }
    };

    for (auto const &s : index)
        if (square[s[0]] == square[s[1]] && square[s[1]] == square[s[2]])
            return WON;

    for (int i = 0; i < 9; i++)
        if (square[i] == i + '0')
            return ONGOING;
    return DRAW;
}

Digits are Contiguous

C++ guarantees that the characters for '0' through '9' are contiguous, so instead of

    if (choice == 1 && square[1] == '1') {
        square[1] = mark;
    } else if (choice == 2 && square[2] == '2') {
        square[2] = mark;
    } else if (choice == 3 && square[3] == '3') {
        square[3] = mark;
    } else if (choice == 4 && square[4] == '4') {
        square[4] = mark;
    } else if (choice == 5 && square[5] == '5') {
        square[5] = mark;
    } else if (choice == 6 && square[6] == '6') {
        square[6] = mark;
    } else if (choice == 7 && square[7] == '7') {
        square[7] = mark;
    } else if (choice == 8 && square[8] == '8') {
        square[8] = mark;
    } else if (choice == 9 && square[9] == '9') {
        square[9] = mark;

...you can do something like:

if (square[choice] == choice + '0')
    square[choice] = mark;

Though you do need to assure that choice is in the correct range before doing this.

More and Simpler Functions

I think I'd take that chunk of code and turn it into a simple function to determine whether a particular square is already used:

bool square_used(int num) { 
    return square[num] != num + '0';
}

Then the code to take a move would look something like:

do {
    cout << "Player " << player << ", enter a number: ";
    cin >> choice;
} while (choice < 0 || choice > 9 || square_used(choice));

That, in turn, probably makes sense as a function in itself that only ever returns a valid move.

Once we've done that, the main loop compresses down to something like:

do { 
    auto choice = get_move(player);
    square[choice] = mark;
    status = checkwin();
    player ^= 1;
} while (status == ONGOING);

if (status == WON)
     std::cout << "Player : " << player << " Won!\n";
else
     std::cout << "Draw\n";
\$\endgroup\$

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.