6
\$\begingroup\$

Recently, I ventured into the realm of C++ programming. I have extensive knowledge in C and C#, but very basic knowledge of C++. I decided to build a brief TicTacToe example to test my knowledge.

It works by typing any comma-separated value (i.e. 2,1) and decides whose turn it is by odds and evens. X always goes first.

Problems: Does not claim a winner. There is no fail-safe for picking the same spot twice. I realize this, but I wanted the semantics over the algorithm. And I don't know of a sure-fire way to find a winner with a vector since I imagine a sea of nested if-statements.

I would love to know what looks legal, and what I should never do again.

Board.h

#include <iostream>
#include <array>
#include <vector>
#include <string>
#include <iterator>

class Board
{
    /*
    ** Protected boolean for checking for winner
    */
    bool gameWon = false;

    /*
    ** Use odd or even to tell whos turn it is
    */
    int turnCount = 0;

public:

    /*
    ** Pair vector for locations of X's and O's
    */
    std::vector<std::pair<int, int>> locations;

    bool GameWon(void){ return gameWon; }
    void DrawBoard(void);
    void NextTurn(void);

private:
    int FindLocation(std::pair<int, int>);

};

Board.cpp

#include "Board.h"

const std::string line = " ------------- ";
const std::string wall = " | ";

/*
** Brute Force Drawing
**
** Iterate over the 9 squares and
** decide whether or not an X belongs
** there or if it's an O. Upon each
** square, check if the pair-location
** (x,y) matches an item in our vector
*/
void Board::DrawBoard()
{
    // Clear screen on windows systems, throws an error
    // on Unix and OS X
    system("CLS");


    std::cout << "Tic-Tac-Toe Console" << std::endl;
    std::cout << line << std::endl;
    for (int i = 0; i < 3; i++)
    {
        for (int j = 0; j < 3; j++)
        {

            bool tileOpen = true;

            std::cout << wall;

            if (locations.size() > 0)
            {

                for (std::pair<int, int> p : locations)
                {

                    if (p.first == i && p.second == j)
                    {
                        tileOpen = false;

                        if (FindLocation(p) % 2 == 0)
                        {
                            std::cout << "X";
                        }
                        else
                        {
                            std::cout << "O";
                        }

                    }
                }
            }
            else
            {
                std::cout << "";
            }

            if (tileOpen == true)
                std::cout << " ";
        }
        std::cout << wall << std::endl;
        std::cout << line << std::endl;
    }
}

/*
** Handles Board Turns
**
** Assume every even turn is X,
** and every odd is O.
*/

void Board::NextTurn()
{
    std::string input;

    if (turnCount % 2 == 0)
        std::cout << " X's turn:" << std::endl;
    else
        std::cout << " O's turn:" << std::endl;
    std::cin >> input;

    locations.push_back(std::pair<int, int>(input[0] - '0' - 1, input[2] - '0' - 1));
    turnCount++;
}

/*
** Vector Find Function
**
** Returns the index of a found object
** in the vector; 0 if otherwise
*/

int Board::FindLocation(std::pair<int, int> p)
{
    std::vector<std::pair<int, int>>::iterator it;
    it = std::find(locations.begin(), locations.end(), p);
    return std::distance(locations.begin(), it);
}

Main.cpp

#include <iostream>
#include "Board.h"

int main()
{
    Board mainBoard;

    /*
    ** Check if there is a winner each cycle
    */
    while (!mainBoard.GameWon())
    {
        /*
        ** Update Board
        */
        mainBoard.DrawBoard();

        /*
        ** Handle Players Turn
        */
        mainBoard.NextTurn();
    }

    /*
    ** Grab a random char so the 
    ** game doesn't immediately exit
    */
    getchar();
}
\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

I would reorganize the code somewhat in order to separate concerns a bit more. Right now, game rules, I/O, etc, are all mixed together.

Board.h

#pragma once

This line should start every header file (the alternative is include guards; see Wikipedia's explanation.

#include <exception>

namespace tictactoe {

All C++ code should go into a namespace; this is essentially a package name. It prevents naming conflicts with other libraries and has other benefits as well.

/* Represents the current state of a tic-tac-toe board. */
class Board {

You should always document your classes.

  public:
    Board();

    enum Square {
      BLANK,
      X_SQUARE,
      O_SQUARE,
    }

    enum Player {
      NEITHER,
      X_PLAYER,
      O_PLAYER,
    }

    /* Throws std::out_of_range if x or y are not in [1,3]. */
    Square GetSquare(int x, int y) const;
    /* Whose turn is it? */
    Player GetCurrentPlayer() const;

Functions that do not modify their class should be marked const.

    /* Updates the board with the current player's move.
     *
     * Returns true if the game is over.
     *
     * Throws std::out_of_range if x or y are not in [1,3].
     * Throws std::invalid_argument if the square is already occupied.
     * Throws game_over if called after the game is over. */
    bool PlayTurn(int x, int y);

    Player GameWinner() const;

  private:
    void UpdateGameWinner();

    Square board[3][3] = {{BLANK, BLANK, BLANK},
                          {BLANK, BLANK, BLANK},
                          {BLANK, BLANK, BLANK}};
    Player current_player;
    Player game_winner;
    bool game_over;
};

struct game_over : public std::exception {
  const char* what() const override;
};

}  // namespace tictactoe

OK, so you see a couple things -- one, Board is not responsible for reading and writing from console. It is the model in a Model-View-Presenter pattern. Two, we're not representing state as you did. There's nothing that says that you have to store data in the same format you receive it (e.g. a sequence of pair<int, int>). Instead, we transform it into something more useful - the actual board pattern. Here we use a two-dimensional array instead of a vector because we know that the size won't change.

BoardView.h

#pragma once

#include <utility>

#include "Board.h"

namespace tictactoe {

/* Interacts with the user in a Tic-Tac-Toe game. */
class BoardView {
  public:
    /* Gets the next move for the given player. */
    virtual std::pair<int, int> GetMove(Board::Player player) = 0;
    /* Reasons a move may have been bad. */
    enum BadMoveReason {
      OUT_OF_RANGE,  // The coordinates were off the board.
      ALREADY_TAKEN,  // The square is already taken.
    };
    /* Tell the user that the move was bad. */
    virtual void DisplayBadMove(BadMoveReason reason) = 0;
    /* Displays the board. */
    virtual void DrawBoard(const Board& board) = 0;
    /* The game is over. Display the winner. */
    virtual void DisplayWinner(Board::Player player) = 0;
};
}  // namespace tictactoe

You can imagine multiple implementations of BoardView; one for a console game, one for a Windows GUI, one for a Unix GUI, etc. But note that BoardView doesn't really need to know anything about the rules of the game - except possibly that coordinates should be between 1 and 3.

I'll leave the rest of the implementation as an exercise to the reader.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ By definition, a #pragma is non-standard and not supported by all compilers. It's an old debate I'll not rehash here, but I use included guards instead which are specified by the standard. \$\endgroup\$
    – Edward
    Commented Jan 5, 2016 at 16:10
  • \$\begingroup\$ @Edward Are include guards necessary for every header file, or just header files used multiple times? \$\endgroup\$ Commented Jan 5, 2016 at 18:14
  • 1
    \$\begingroup\$ If a header is only used once, there's not much point to creating it; therefore all headers should have include guards. \$\endgroup\$
    – Edward
    Commented Jan 5, 2016 at 18:16
2
\$\begingroup\$

I would also structure the code quite differently, but there is at least one other answer already addressing that. Instead, I will focus mostly on implementation details.

Some comments are too verbose

To me, many of your comments are too verbose. In general, I'd want to read comments that explain "why", and not "what". For instance, it is absolutely clear from good naming, that a line like while (!mainBoard.GameWon()) executes the main loop. I think adding a comment emphasizing this in fact hurts readability.

On interfaces

If you have a (member) function that takes no arguments, write it as void my_function();. That is, omit void from the argument list, it adds no information and likely only irritates a C++-programmer reading the code. Also, and this is important, mark every member function const that does not modify the state of the object. Typically, if you are printing information or something similar, you are not modifying the object. As a reader or a user of your interface, I fully expect such a member function to be const. In fact, all of your 4 member functions seem like they can be const, except for the one that advances the game. By the way, including array, string, and iterator seem totally unnecessary for the header file. Include them in the source file.

Use even more const variables

It's good that you are using const std::string to represent walls and such. I would probably wrap these two variables inside an anonymous namespace, thus restricting them to the particular source file only. There is no need to access them from the outside, after all. Similarly, why not make "X" and "O" constants as well? Maybe you'd like to change the symbols in the future.

Implementation details

Consider the following line:

if (locations.size() > 0)

You are testing for the emptiness of a container. This is not the most idiomatic way of doing it. Use always the most appropriate member function available. In this case, the line should be if(!locations.empty()). For a vector, this might not have any effect. But for say std::list, it might. Bottom line is that you should not make any assumptions as to how a container is actually implemented. It could be that empty() takes constant time, while querying for its size takes time linear in the number of elements in the container. So be careful, or face unexpected performance consequences :-)

Instead of constructing a std::pair like you do, you might want to be aware of std::make_pair. I would likely also write your FindLocation function so that it only consists of a single return-statement (by the way, you could also operate with const_iterators here, as you are not modifying anything. Moreover, you don't want to modify anything).

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the information. As for the #includes in the header file, is there any standard to follow regarding where they should be located? I figured if I used them in the source file, I might as well put them in the header so I only need one #include. I guess that decreases readability though. \$\endgroup\$ Commented Jan 5, 2016 at 18:08
  • \$\begingroup\$ @user3791725 Basically, you should have includes at the latest possible moment. It is also a habit for me to include my own headers first, then the standard ones. Someone argued for this by saying "if something fails, it's likely your own header, so you'll get to errors faster". For the header, include whatever the header itself is using. You should also know about forward declaration: this is a way of introducing necessary classes for a header. Proper handling of includes possibly saves you compilation time, and also avoids some compiler or linker errors. \$\endgroup\$
    – Juho
    Commented Jan 5, 2016 at 18:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.