4
\$\begingroup\$

It's time for my second simple game - Tic Tac Toc. I checked it on my Linux and it's working. For compiling it I used g++ with -std=c++11. I will be glad for any advice for the future.

#include <iostream>
#include <cstdlib>
#include <unistd.h>

class Game_Board
{
public:
    Game_Board();

private:
    enum Player{cross=1, circle=2};
    Player which_player = cross;
    int table_board[3][3];
    bool end = false;

//drawing board
    void draw_board(int const table_board[3][3])
    {
        system("clear");
        for (int i = 0; i<3; i++)
        {
            std::cout << "\n";
            for (int j = 0; j<3; j++)
            {
            if(table_board[j][i]==1)
            {
                std::cout << " _X_ ";
            }
            else if(table_board[j][i]==2)
            {
                std::cout << " _O_ ";
            } else
            {
            std::cout << " ___ ";
            }
            }
            std::cout << "\n\n";
        }
    }

//reading player move
    void read_move()
    {
        int *p_moveX = new int;
        int *p_moveY = new int;
        std::cout << "\nChoose a field (Player " << which_player << "): ";
        std::cin >> *p_moveX >> *p_moveY;
        write_move(*p_moveX, *p_moveY, which_player);
        delete p_moveX; p_moveX = NULL;
        delete p_moveY; p_moveY = NULL;
    }

//saving player's move to table
    void write_move( int X, int Y, int plr_number)
    {
        if(table_board[X-1][Y-1]!=0 || X<=0 || Y<=0 || X>3 || Y>3)
        {
            std::cout << "You can't do this!";
            read_move();
        }
        else
        {
        table_board[X-1][Y-1] = plr_number;
        }
    }

//checking end of game
    void check_winner()
    {
    int *end_counter = new int;
    *end_counter = 0;
        for( int i = 0; i<3; i++)
        {
            if((table_board[i][0] & table_board[i][1] & table_board[i][2])!=0 ||(table_board[0][i] & table_board[1][i] & table_board[2][i])!=0 || (table_board[i][0] & table_board[i+1][1] & table_board[i+2][2])!=0 || (table_board[i][2] & table_board[i+1][1] & table_board[i+2][0])!=0)
            {
                switch(which_player)
                {
                    case 1:
                    draw_board(table_board);
                    std::cout << "Player 1 wins!\n";
                    end = true;
                    break;

                    case 2:
                    draw_board(table_board);
                    std::cout << "Player 2 wins!\n";
                    end = true;
                    break;
                }

            }
        }
        for( int i = 0; i<3; i++)
        {
            if(table_board[i][0]!=0 && table_board[i][1]!=0 && table_board[i][2]!=0)
            {
                    *end_counter+=1;
                    if(*end_counter==3)
                    {
                    draw_board(table_board);
                    std::cout << "Only God wins!\n";
                    end = true;
                    }
            }
        }
        delete end_counter;
        end_counter = NULL;
    }

//choosing player
    void choose_player()
    {
        if(which_player==1)
        {
            which_player = circle;
        } else {
            which_player = cross;
        }
    }

//clearing table
    void clear_table()
    {
        for(int i=0; i<3; i++)
        {
            for(int j=0; j<3; j++)
            {
                table_board[i][j]=0;
            }
        }
    }
};

Game_Board::Game_Board()
{
    clear_table();
    while(!end)
    {
    draw_board(table_board);
    read_move();
    check_winner();
    choose_player();
    }
}

int main()
{
    Game_Board game;
    return 0;
}
\$\endgroup\$
4
\$\begingroup\$

I would recommend making a main game loop. Running a loop in a constructor breaks the fundamental contract of a constructor: to construct something. This change will also allow you to easily restart the game and keep track of score if you wish. With that said, adding a StartGame, RestartGame and EndGame functionality may help organize things.

I tend to shy away from enums. If I find myself needing an enum, I always end up realizing I actually want a class. This follows the S in SOLID: single responsibility.

There is a line of code in check_winner that goes well over 100 columns. I would wrap the line after every few boolean operators in an if statement like this.

if((table_board[i][0] & table_board[i][1] & table_board[i][2])!=0
  || (table_board[0][i] & table_board[1][i] & table_board[2][i])!=0
  || (table_board[i][0] & table_board[i+1][1] & table_board[i+2][2])!=0
  || (table_board[i][2] & table_board[i+1][1] & table_board[i+2][0])!=0)
{
...
}

You seem to be using the new operator where you don't need to. All of this can be done on the stack with less lines of code and less work for the processor. Every time you call new, the CPU has to find new space on the heap, construct the datatype and return the pointer to the memory address. Considering how small this program is, it's likely all your data could fit in the registry directly on the CPU.

void read_move()
{
    int moveX = 0;
    int moveY = 0;
    std::cout << "\nChoose a field (Player " << which_player << "): ";
    std::cin >> moveX >> moveY;
    write_move(moveX, moveY, which_player);
}

check_winner also has a similar problem.

draw_board does not need to take in a table_board because it is already accessible as a member, also I would avoid giving function parameter the same names as members. I'd recommend putting a lowercase 'm' before your member variables and using pascal case.

After the appropriate code changes, you shouldn't have any pointers in your code, but just as a side note:

nullptr is now the standard for assigning pointers to null in C++11. nullptr is of type std::nullptr_t where NULL is an int.

\$\endgroup\$
  • 1
    \$\begingroup\$ nullptr is of type nullptr_t. \$\endgroup\$ – 1201ProgramAlarm Jan 21 '18 at 1:54
  • \$\begingroup\$ Thank you for the correction. Yes, it is of type std::nullptr_t. \$\endgroup\$ – aj.toulan Jan 21 '18 at 3:08
3
\$\begingroup\$

Adding on to the review from aj.toulan, here are some other suggestions:

  1. Your default constructor currently has nothing to do, since you have done a good job using default member initializers in your class declaration. Kudos!

  2. I would suggest adding an istream& and ostream& member to the class to use for reading and writing. This would let you write tests using string streams.

  3. As mentioned, your main function basically says "There is a game board." The constructor is not the place to be implementing game play. Better if you wrote a play() method or something, so that main could say, "There is a game board(that reads from cin, and writes to cout). Play one game on that board."

    int main()
    {
        TicTacToe game(std::cin, std::cout);
        game.play();
        return 0;
    }
    

    Your current constructor makes a perfect play function.

  4. Why do you need to clear_table after initialization? Are some of your members not properly initialized? If only there were some way to specify the initial value of an aggregate type...

  5. enum values are int, which conveniently is large enough to hold a char. Instead of creating enumerated values that are integers, why not create values that are characters?

    enum {
        cross = 'X',
        circle = 'O',
    };
    
  6. Which brings up the question of why you need a which_player() function to decode circles and crosses from an integer. You don't, if you put the character value in the enum.

  7. Instead of reading in the move, not validating it, then checking for failure when you write the move, consider writing a loop (while (true) or do ... while (!valid_move(x,y)); or something) that will not exit the read function until a valid move is entered.

  8. Your check_winner function contains undefined behavior. You are allowed to point 1 element past the end of an array, but not more. And you certainly are not allowed to access outside the bounds of an array.

    I suggest that you create a table, where each row is a "valid winning line." That is, each row of the table contains the 3 (x,y) addresses of a line of winning points. If all of the points in a row contain the same value, that player wins. You can iterate over the rows, then iterate the values, looking for the winner.

\$\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.