Ggthjy

Last week I wrote a C++ program on Ultimate Tic Tac Toe. The issue was that the program was a bit lengthy (~230 lines) and it is needed to be around 150 lines.

I absolutely am not asking you to go through the whole code. Just skimming through might reveal possible contractions to code, and tips in general to write short but readable C++ code.

Code

#include <iostream>

#include <cstdlib>

#include <fstream>

#include <string>



using namespace std;



inline int X (int pos) {

    return pos / 3;

}



inline int Y (int pos) {

    return pos % 3 - 1;

}



string rule(80, '_');



class Grid

{

private:

    char subgrid[3][3];

    char left, right;



public:

    Grid(){}

    void set (int x, int y, char cell);

    char get (int x, int y);

};



void Grid::set (int x, int y, char cell) {

    subgrid[x][y] = cell;

}



char Grid::get(int x, int y) {

    return subgrid[x][y];

}



class Game

{

private:

    Grid grid[3][3];

    char player;

    size_t cur;



public:

    Game();

    void display(); 

    void play();

    void input (int& g);

    bool checkWin (Grid grid);

    void showScore();

};



Game::Game() {

    for (size_t i = 0; i < 3; ++i)

    for (size_t j = 0; j < 3; ++j)

    for (size_t k = 0; k < 3; ++k)

    for (size_t l = 0; l < 3; ++l)

    {

        grid[i][j].set(k, l, '.');

    }

    player = 'x';

    cur = 0;

}



void Game::play()

{

    int g, s;

    display();

    while (1) {

        display();

        cout << "n  Player " << player << " - Enter Grid: ";

        cin >> g;

        if (g > 0 && g < 10) {

            break;

        }

        display();

        cout << "n  Try again.";

        cin.get();

    }

    s = g;

    while (1) {

        display();

        if (checkWin(grid[X(cur)][Y(cur)])) {

            display();

            cout << "n  Player " << player << " won!";

            cin.get();

            cin.ignore();

            break;

        }

        player = player == 'x' ? 'o' : 'x';

        cur = g;

        input(g);

    }



}



void Game::display()

{

    system("cls");

    cout << "n  ULTIMATE TIC TAC TOEn" << rule;



    for (size_t i = 0; i < 3; ++i)

    {

        for (size_t k = 0; k < 3; ++k)

        {

            cout << "n";

            char left, right;

            left = right = ' ';



            for (size_t j = 0; j < 3; ++j)

            {

                if (k == 1)

                {

                    if (3*i + j + 1  == cur) {

                        left = '>';

                        right = '<';

                    }

                    else {

                        left = right = ' ';

                    }

                }

                cout << "  " << left << " ";

                for (size_t l = 0; l < 3; ++l) {

                    cout << grid[i][j].get(k, l) << " ";

                }

                cout << right;

            }

        }

        cout << "nn";

    }

    cout << "n";

}



void Game::input(int& g)

{



    int s;

    while (1) {

        display();

        cout << "n  Player " << player << " - Enter subgrid: ";

        cin >> s;

        if (s > 0 && s < 10)

        {

            if (grid[X(g)][Y(g)].get(X(s), Y(s)) == '.') {

                break;

            }

        }

        display();

        cout << "n  Try again.";

        cin.ignore(); cin.get();

    }

    grid[X(g)][Y(g)].set(X(s), Y(s), player);

    g = s;

}



bool Game::checkWin(Grid grid)

{

    char p = player;

    int row = 1, col = 1, main_diag = 1, anti_diag = 1;

    for (size_t i = 0; i < 3; ++i)

    {

        row = col = 1;

        if (grid.get(i, 3-1-i) != p) {

            anti_diag = 0;

        }

        if (grid.get(i, i) != p) {

            row = col = main_diag = 0;

        }



        else {

            for (size_t j = 0; j < 3; ++j)

            {

                if (grid.get(i, j) != p) {

                    row = 0;

                }

                if (grid.get(j, i) != p) {

                    col = 0;

                }

            }

        }



        if (row || col) {

            return 1;

        }

    }



    if (main_diag || anti_diag) {

        return 1;

    }

    return 0;

}



int main()

{

    Game game;

    game.display();

    cout << "n  Welcome to Ultimate Tic Tac Toe." <<

        "n  Press Enter to start.";

    cin.get();



    int input, error = 0;

    enum menu { play = 1, scores, quit };

    do {

        game.display();

        if (error) {

            cout << "  Invalid option. Try again.n";

            error = 0;

        }

        else {

            cout << "  Select an option: n";

        }

        cout << "    1) Playn    2) Scoresn    3) Quitn" << "n> ";

        cin >> input;

        switch (input) {

            case play:

                game.play(); break;

            case scores:

                //showScores();

                break;

            case quit:

                std::exit(0);

            default:

                error = 1;

        }

        system("cls");

    } while (error);



    system("pause");

    return 0;

}

Here are some things that may help you improve your code.

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. For this program, I'd advocate removing it everywhere and using the std:: prefix where needed.

Don't use system("cls")

There are two reasons not to use system("cls") or system("pause"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a seperate functions cls() and pause() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++. For example, if your terminal supports ANSI Escape sequences, you could use this:

void cls()

{

    std::cout << "x1b[2J";

}

Eliminate unused variables

The variable s in your Game::play() code is defined but never used. Also, left and right within Grid are never used. Since unused variables are a sign of poor code quality, you should seek to eliminate them. Your compiler is probably smart enough to warn you about such things if you know how to ask it to do so.

Use rational default constructors

If you provide a constructor for Grid that initializes its contents to all ., then the constructor for Game is shorter and much more readable.

Grid() : subgrid{'.','.','.','.','.','.','.','.','.'} {}

Eliminate global variables

In this case, the only global variable is rule which is only used once. I'd move it to within Game::display() and declare it like this:

static const std::string rule(80, '_');

Delegate more to the subclass

The Grid object is not doing very much. It could be assisting more in the display() and checkWin() tasks in particular.

Eliminate unused #includes

The cstdlib library is not required if you change std::exit() to simply return in main.

Eliminate unimplemented code

The showScore() code is missing and is never called anyway. It could simply be deleted, along with the associated case statement and menu option.

Use const where practical

Member functions that don't alter the underlying object should be declared const.

Use standard structures and algorithms

One important and useful way to simplify code is to make better use of existing library code. In particular, the Standard Template Library (STL) would be very helpful here. For instance, you could use a std:array instead of a plain C array to represent each grid. Internally, the representation could be std::array<char, 9> and translation from x and y coordinates could be done by member functions. As an example:

class Grid {

private:

    std::array<char, 9> subgrid;



public:

    Grid() : subgrid{'.','.','.','.','.','.','.','.','.'} {}

    void set (int i, char cell) { subgrid[i] = cell; }

    char get (int i) const { return subgrid[i]; }

    char get (int x, int y) const { return subgrid[x+3*y]; }

    bool checkWin(char player) const {

        // check for col and row wins

        for (int i=0; i < 3; ++i) {

            if((player == get(i, 0) && 

                player == get(i, 1) &&

                player == get(i, 2)) || 

               (player == get(0, i) && 

                player == get(1, i) &&

                player == get(2, i))) {

                    return true;

            }

        }

        // check diagonals

        return (player == get(1,1) && 

               ((player == get(0,0) && player == get(2,2))

             || (player == get(0,2) && player == get(2,0))));

    }

    std::string line(int linenum) const {

        std::string ret;

        if (linenum >= 0 && linenum < 3) {

            for (int i=0; i<3; ++i) {

                ret += get(i, linenum);

            }

        }

        return ret; 

    }

};

Now the Game::display() is much neater and smaller:

void Game::display()

{

    cls();

    static const std::string rule(80, '_');

    std::cout << "n  ULTIMATE TIC TAC TOEn" << rule << 'n';



    for (int i=0; i < 9; i += 3) {

        for (int line = 0; line < 3; ++line) {

            for (int j=0; j < 3; ++j) {

                if (line == 1 && (cur-1 == i+j)) {

                    std::cout << " > " << grid[i+j].line(line) << " < ";

                } else {

                    std::cout << "   " << grid[i+j].line(line) << "   ";

                }

            }

            std::cout << "n";

        }

        std::cout << "nn";

    }

}

Avoid breaking loops

Rather than use break to exit a loop, it's usually better to simply declare the actual loop exit at the top so that someone reading your code doesn't have to wonder where the actual exit lies. For example, one way to rewrite Game::input is like this:

void Game::input(int& g)

{

        int s;

        bool badinput = false;

        for (s = 0; s < 1 || s > 9 || grid[g-1].get(s-1) != '.'; badinput = true) {

                display();

                if (badinput) {

                    std::cout << "Try again";

                }

                std::cout << "n  Player " << player << " - Enter subgrid: ";

                std::cin >> s;

        }

        grid[g-1].set(s-1, player);

        g = s;

}

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments: "I didn't know that." or "That's bad advice!" My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard. For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

[...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!) In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.

Read about Alan Turing a very fascinating man - the very first computer scientist, if you will. In his quest to break every single message sent from the German Enigma Machine he realized there are so many possibilities that it would take over 20,000 years to test each one daily.

Taking this approach to your tic-tac-toe game:

• There are only eight possibilities to a win.


• There must be 'Three in a row' to be a win.

If there is no win -- there is no loser either.

1. We have no reason to test for a win until at least one player has three marks on the board.




2. If we test 0,0 for 'empty' and it is indeed empty, we can eliminate three of the possibilities -- leaving only five.




3. if we test 1,1 for empty, and it too is empty, we can eliminate two more possibilities, leaving only three to be tested.




4. If we test 2,2 for empty and it too is empty, we can eliminate the remaining three possibilities.




5. However, if we come across a square that is not empty - then we only need to test the possibilities associated to that particular square - not all eight.

But again, we have no reason to test until a possible win is even possible - so until one player had three turns there is no need to test for a win.
We can test any of the three spaces in any one diagonal - just use the same diagonal for all three though.

Using this approach, we only test three spaces to determine if a win is possible, and only after the first player's third turn. If that first player wins after four turns we only actually test some possible solutions two times.