6
\$\begingroup\$

I've decided to learn C++, coming from Python, and this is my first program. I would love some feedback on it.

It's split up into two header files and one .cpp file:

Board.h

class Board {

public:

    std::string board[7][4] = {
        { "\n\n-------------\n" },
        { "| 1 ", "| 2 ",  "| 3 ", "|\n" },
        { "----+---+----\n" },
        { "| 4 ", "| 5 ",  "| 6 ", "|\n" },
        { "----+---+----\n" },
        { "| 7 ", "| 8 ",  "| 9 ", "|\n" },
        { "-------------\n" }
    };

    void displayBoard(); // Displays the board[2D Array]

} board;

void Board::displayBoard()
{
    for (int x = 0; x < 7; x++) // Row of the 2D array
        for (int i = 0; i < 4; i++) // Column of the 2D array
            std::cout << board[x][i];
}

Player.h

class Player {

private:

    int in, row, col;
    std::string peice, player;

public:

    Player(std::string x); // Sets up peice, depending on parameter
    int move();           // Gets input and makes move
    bool winner();       // Checks for winner

} p1("X"), p2("O");




// CLASS FUNCTIONS

Player::Player(std::string x)
{
    player = x;
    peice = player == "X" ? "| X " : "| O ";
}

int Player::move()
{
beginning:

    while (!(in > 0 && in < 10)) // Only accpets values between 1-9
    {
        std::cout << "\nPlayer " << player << " enter position: ";
        std::cin >> in;
    }

    row = in > 3 && in < 7 ? 3 : 1;                    // Sets row
    row = in > 6 && in < 10 ? 5 : row;

    col = in == 2 || in == 5 || in == 8 ? 1 : 0;       // Sets Column 
    col = in == 3 || in == 6 || in == 9 ? 2 : col;

    in = 0; // Sets back to 0 so the while expression will work again

    if (board.board[row][col] == "| X " || board.board[row][col] == "| O ")
    { // If space has already been taken, ask for input again
        goto beginning;
    }


    board.board[row][col] = peice;  // Convert position from board into the peice
    system("cls");                 // Clears everything in the window
    board.displayBoard();         // Display Board

    if (winner() == true) { return 1; } // Checks if this player has won
}

bool Player::winner()
{
    for (int x = 1; x < 6; x += 2)
    {
        if (board.board[x][0] == peice && board.board[x][1] == peice && board.board[x][2] == peice || // Horizontal
            board.board[1][x - 1] == peice && board.board[3][x - 1] == peice && board.board[5][x - 1] == peice || // Verical
            board.board[1][x - 1] == peice && board.board[3][1] == peice && board.board[5][x == 1 ? 2 : 0] == peice) // Diagonal
        {
            std::cout << "\nPlayer " << player << " has won\n";
            return true;
        }
    }
}

TicTacToe.cpp

// Tic Tac Toe - TCG

#include <iostream>
#include <string>

#include "Board.h"
#include "Player.h"

int main()
{
    std::cout << "Welcome to Tic Tac Toe";
    board.displayBoard();

    for (int x = 0; x < 6; x++)
    {
        if (p1.move() == 1) { break; } // Player X

        if (x == 4) // If there is no winner up until the end of the game; end game
        {
            std::cout << "\n\n\nThe match was a draw\n";
            break;
        }

        if (p2.move() == 1) { break; } // Player O
    }
}

Here is the program compiled and running:

Welcome to Tic Tac Toe

-------------
| 1 | 2 | 3 |
----+---+----
| 4 | 5 | 6 |
----+---+----
| 7 | 8 | 9 |
-------------

Player X enter position:

Here is the updated version.

\$\endgroup\$
  • \$\begingroup\$ What should I do when someone answers my question? Do not change the code in the question after receiving an answer. Incorporating advice from an answer into the question violates the question-and-answer nature of this site. \$\endgroup\$ – t3chb0t Oct 31 '16 at 10:09
26
\$\begingroup\$

Here are some observations and suggestions that may help you improve your program.

Check your spelling

If you run a spell check on your comments, you'll find a number of things such as "peice" instead of "piece" and "accpets" instead of "accepts". Since your code is nicely commented, it's worth the extra step to eliminate spelling errors.

Separate interface from implementation

The interface goes into a header file and the implementation (that is, everything that actually emits bytes including all functions and data) should be in a separate .cpp file. The reason is that you might have multiple source files including the .h file but only one instance of the corresponding .cpp file. In other words, split your existing Player.h file into a Player.h file and a Player.cpp file and do the same with Board.h.

Always return an appropriate value

Your Player::move() routine has control paths that cause it to end without returning any int value. This is an error and should be fixed.

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";
}

Use appropriate #includes

The Player class uses a std::string as part of its interface, but there are no #includes in that file. Each file should have only and exactly the required #includes. In this case, it needs #include <string> within the header.

Eliminate "magic numbers"

In a number of cases, the code uses "magic numbers" such as 10 and 6 that have no obvious meaning. These would be better as named constants if they're used more than once.

Avoid the use of global variables

In this code p1, p2 and board are all global variables. It's generally better to explicitly pass variables your function will need or declare them within the appropriately smallest possible scope rather than using the vague implicit linkage of a global variable. For example, the main function could instead have three local variables like this:

Board board;
Player p1("X"); 
Player p2("O");

If the Player member functions need access to the board, then pass a reference to the board as needed. For example:

p1.move(board)

Use include guards

There should be an include guard in each .h file. That is, start the file with:

#ifndef BOARD_H
#define BOARD_H
// file contents go here
#endif // BOARD_H

Fix the bug

The way it's currently written, if player 1 wins by getting all the spaces in the middle column, it is not recognized by the program as a win. I'll leave it to you to figure out what's wrong and fix it.

Simplify expressions

The Player code currently contains this line:

if (winner() == true) { return 1; } // Checks if this player has won

This has three problems. First, as mentioned above, the function should also return something if the expression is false. Second, if the return from the move function is interpreted as a binary value, the function should be declared as returning bool rather than int. Third, this can be simplifed to this:

return winner(board);

Which takes care of all three problems and also assumes that the function has been modified to pass board instead of using a global.

Use const where practical

In the Board::display routine, the underlying Board is not altered. Make this explicit by declaring that routine const as:

void displayBoard() const; // Displays the board[2D Array]

Rethink your class design

The Player classes freely access the internals of the Board class and the Board class member data is public, meaning that any code can modify it. That's not good object design. Better would be to think clearly about the separation between the Board and Player objects and to provide a clean separation and interface between the two.

Separate responsibilities

The Model-View-Controller design pattern is often useful for programs like this. Because the view in this case is essentially just printing the board to std::cout, we can simplify a bit and just have a model, the Board class, and perhaps a pair of controllers, the Player class.

Use meaningful variable names

Your Board and Player class are descriptive and good enough, but the variable names are not so good. In particular the x variable in main is not at all descriptive of the content. A better name might be moveCount.

Declare the loop exit condition at the top

The for loop inside main currently says this:

for (int x = 0; x < 6; x++)

Reading that line, we would conclude that the play continues until x >= 6. However, within the loop is are three breaks that occurs if one player has won or if it's a draw. Rather than forcing the reader of the code to examine every line, it's better if you simply declare loop exit conditions completely and honestly at the top.

while (!board.full() && !p1.move(board) && !p2.move(board)) {   
    // play until either draw or one player wins
}

Avoid using goto

Having a proliferation of goto statements is usually a sign of bad design. Better would be to eliminate them entirely -- it makes the code easier to follow and less error-prone. In this code, you could very easily use a loop instead:

do {
    // try to apply the move
} while (board.board[row][col] == "| X " || board.board[row][col] == "| O ");

Separate I/O from logic

The Player::move routine prints a prompt, gets input from the user and then applies that to the board. It would be better to separate I/O from the game logic so that the game is cleanly separated from the I/O. This would, for example, make it much easier to convert the game into a GUI version.

Use a better data structure

The Board class uses an array of printable strings as its data member. There's nothing inherently wrong with this, but that only simplifies the printing of the board at the expense of every other use. Instead, choose a more natural data structure, such as a 3x3 array, and you'll find that the calculations are much easier to write correctly.

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

Here are my comments:

Suggest distinguishing between logical and visual representation. The logical representation of tic-tac-toe is a 3x3 array of values, visual can be anything you like. In other words, I would have the board as:

static const unsigned int BoardDimensionSize = 3
enum class CellValues = {X, O, empty}
using PlayBoard = std::vector<std::vector<CellValues>>
PlayBoard theBoard;

Then have a method initializeBoard(PlayBoard &board) which sets vector sizes to BoardDimensionSize and values to CellValues::empty . (We could use an initializer directly on theBoard.

displayBoard can accept a PlayBoard and visualize it in a similar way to the your original board field.

2) goto is out of place here. The structure of the main loop is simple:

  • while there are cells that are not empty
  • Accept a move for player X
  • Check victory condition. If win, break from the loop
  • Do the same for player O

So, you need to implement methods:

  • isValidMove
  • areCellsFree
  • setMoveAndValidateWin

And plug them in the right places above. I'll finish for now, since it's better to fix flow issues before going to C++ specifics.

\$\endgroup\$
  • \$\begingroup\$ I've just started out and don't know how to use the vector library. I will get on that soon. \$\endgroup\$ – TCG Oct 30 '16 at 16:28
  • \$\begingroup\$ It's not strictly a library - there is a whole set of containers, which are part of the standard. See here: cplusplus.com/reference/stl \$\endgroup\$ – RomanK Oct 30 '16 at 16:39
  • \$\begingroup\$ I think it would be better to create std::array<std::array<CellValues, BoardDimensionsSize>, BoardDimensionSize> by making BoardDimensionSize constexpr. Also, initializeBoard() is anti pattern. \$\endgroup\$ – Incomputable Oct 30 '16 at 19:45
  • \$\begingroup\$ Yes, that is slightly better. initializeBoard can easily become a constructor of the class Board. Either way, the problems in the original code are way more basic. \$\endgroup\$ – RomanK Oct 30 '16 at 20:36

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.