3
\$\begingroup\$

Just created my first C++ project, and wondering if I could get some feedback on where to improve? I might swap from arrays to vectors and add alpha beta pruning later, but wanted to first see where I can improve on the base code.

#include <iostream>
#include <cmath>

const char PLAYER_ONE_MARKER = 'X';
const char PLAYER_TWO_MARKER = 'O';
const char UNFILLED_MARKER = ' ';

char board[3][3];
const int MAX_TURNS = sizeof(board);

void drawBoard();
void placeMarker(bool isPlayerOne, int row, int col);
bool checkValidInput(int row, int col);
bool checkWon(int row, int col);

int  main()
{
    int row, col;
    bool isPlayerOneTurn = true;
    int turn = 0;
    bool isGameRunning = true;
    bool isValidInput;

    // fills board with unfilled_marker
    std::fill(&board[0][0], &board[0][0] + sizeof(board) / sizeof(board[0][0]), ' ');

    while (isGameRunning)
    {
        // loops until we get a valid set of input from player
        isValidInput = false;
        do {
            std::cout << "Player " << (isPlayerOneTurn ? "1" : "2") << ", what is your move?\n";
            std::cin >> row >> col;

            isValidInput = !std::cin.fail() && checkValidInput(row, col);
            std::cin.clear();
            std::cin.ignore(10, '\n');

        } while (!isValidInput);
    
        placeMarker(isPlayerOneTurn, row, col);

        // after we place marker, check if player has won
        if (checkWon(row, col)) {
            std::cout << "Player " << (isPlayerOneTurn ? "1" : "2") << " has won!\n";
            isGameRunning = false;
        }
        else if (++turn >= MAX_TURNS) {
            std::cout << "It's a draw\n";
            isGameRunning = false;
        }
        drawBoard();

        // switches to other player's turn
        isPlayerOneTurn = !isPlayerOneTurn;
    }
    
    return 0;
}

void drawBoard()
{
    int rows = sizeof(board) / sizeof(board[0]);
    for (int i = 0; i < rows; ++i)
    {
        std::cout << board[i][0] << "  |  " << board[i][1] <<
            "  |  " << board[i][2] << std::endl;
        
        // draws line between rows
        if (i < rows - 1)
        {
            std::cout << "---------------\n";
        }
    }

}

bool checkValidInput(int row, int col)
{
    if (row < 0 || col < 0 || row > 2 || col > 2) return false;

    // checks if position supplied is already marked
    return board[row][col] == UNFILLED_MARKER;
}

void placeMarker(bool isPlayerOne, int row, int col)
{
    char marker = isPlayerOne ? PLAYER_ONE_MARKER : PLAYER_TWO_MARKER;
    board[row][col] = marker;
}

// given the move played, check if user has won
bool checkWon(int row, int col)
{
    // first check for horizontal/vertical victory cases
    if (board[row][0] == board[row][1] && board[row][1] == board[row][2]) return true;
    if (board[0][col] == board[1][col] && board[1][col] == board[2][col]) return true;

    // for top-left to bottom-right diagonals, row and col must be the same
    // for instance (0, 0) - (1, 1) - (2, 2)
    // for bottom left to top-right, one of th terms must be (0, 2) or (2, 0)
    // hence we can just use abs(row - col) == 2 to test
    if (row == col || std::abs(row - col) == 2) {
        if (board[0][0] == board[1][1] && board[1][1] == board[2][2]
            && board[1][1] != UNFILLED_MARKER) return true;
        if (board[2][0] == board[1][1] && board[1][1] == board[0][2]
            && board[1][1] != UNFILLED_MARKER) return true;
    }

    return false;
}
\$\endgroup\$
1
\$\begingroup\$

This is not at all a bad effort for someone new to the C++ language. Here are some things that may help you improve your program.

Use the required #includes

The code uses std::fill which means that it should #include <algorithm>. It was not difficult to infer, but it helps reviewers if the code is complete.

Eliminate global variables where practical

The code declares and uses a global variable, board. Global variables obfuscate the actual dependencies within code and make maintainance and understanding of the code that much more difficult. It also makes the code harder to reuse. For all of these reasons, it's generally far preferable to eliminate global variables and to instead pass pointers to them. That way the linkage is explicit and may be altered more easily if needed. One could create the board within main and then pass a pointer or reference to it to the other routines that need it, or better yet, see the next suggestion.

Use a C++ object

Rather than using the global variable board and then having separate functions that manipulate that variable such as drawBoard() and placeMarker() in an object-oriented language such as C++ we can gather these together into an object. That might look something like this:

class Board {
public:
    Board();
    void drawBoard();
    void placeMarker(bool isPlayerOne, int row, int col);
    bool checkValidInput(int row, int col);
    bool checkWon(int row, int col);
    static constexpr char PLAYER_ONE_MARKER = 'X';
    static constexpr char PLAYER_TWO_MARKER = 'O';
    static constexpr char UNFILLED_MARKER = ' ';
    static constexpr size_t numrows = 3;
    static constexpr size_t numcols = 3;
    static constexpr unsigned max_turns = numrows * numcols;
private:
    char board[3][3];
};

Eliminate "magic numbers"

There are a few numbers in the code, such as 3 and 2 that have a specific meaning in their particular context. By using named constants such as numrows or numcols, the program becomes easier to read and maintain.

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Consider using a standard container

There is little reason in modern C++ to use a plain array rather than a standard container such as std::array. I'd suggest that board could be this instead:

std::array<char,max_turns> board;

The initialization would then be much more easily written:

std::fill(board.begin(), board.end(), UNFILLED_MARKER);

Note that this form would require the conversion of row,column coordinates but if this is placed in an object, that detail can easily be isolated to within the object.

Don't use ALL_CAPS for constants

The use ALL_CAPS for constants defined as macros has been common practice for decades. To avoid misleading readers of your program, non-macro constants should not be ALL_CAPS. See ES.9 for details.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you so much for your extensive feedback, and this is awesome. I'll work on refactoring my code based on your tips, especially OOP. Also, really interesting to read that C++ doesn't like all_caps for constants, will keep that in mind! \$\endgroup\$ – Dan Tang Mar 23 at 10:16

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.