4
\$\begingroup\$

I have finally found some time to redo the last tic tac toe game I posted. I have gotten rid of "Magic Numbers" as well as any typos(if their are any please point them out but I think i rid myself of them...hopefully). I have updated the way the code is implemented as well as added a header titled "Constants". This choice was to get rid of my magic number issues and if it is was a poor idea please correct me on it and show me the proper way to create cross file constants. (I tried to use constants as differing types of error codes. Is this a good idea?) One more thing, should I implement the input type checking in my Data class or leave it where it is to match MVC design pattern. (If my design does not implement this pattern please tell me how I should do it.)

Constants.h

#ifndef CONSTANTS
#define CONSTANTS
enum boardSpaces : size_t {
    space1 = 0, space2 = 1, space3 = 2, //used to access board spaces in container
    space4 = 3, space5 = 4, space6 = 5,
    space7 = 6, space8 = 7, space9 = 8
};
 const int errRecognize = 0; //error unrecognizable input i.e. number or char out of range
const int errType = 1; //error data type of user input faild
const int errBoard = 2; //error space of board has been marked
#endif

Data.h

#ifndef DATA
#define DATA

#include <string>

class Data
{
    std::string boardData;
    const char player1;
    const char player2;
public:
    const int MAX_TURNS = 9;
    const int board_dim = 3; //dimensions 3X3
    Data();
    ~Data() = default;
    std::string printBoard() const;
    void markBoard(const size_t&,const char&);
    void gameReset();
    bool checkWin(const char&) const;
    bool checkCatsGame(const int&) const;
    char boardSpaceValue(const size_t&) const;
    char player1Mark() const;
    char player2Mark() const;
};

#endif

Data.cpp

#include "Data.h"
#include "Constants.h"

Data::Data(): player1('X'), player2('O')
{
    boardData = "123456789";
}

void Data::markBoard(const size_t &position,const char &playerMark) {
    boardData[position-1] = playerMark;
}

std::string Data::printBoard() const{
    return boardData;
}

//returns value of a praticular space on board
char Data::boardSpaceValue(const size_t &index) const {
    return boardData[index-1];
}

char Data::player1Mark() const{
    return player1;
}

char Data::player2Mark() const{
    return player2;
}

bool Data::checkWin(const char &mark) const{
    //check columns and rows for win
    //code came from Edward@ codereview
    for (unsigned i = 0; i < board_dim; ++i) {
        bool rowwin = true;
        bool colwin = true;
        for (unsigned j = 0; j < board_dim; ++j) {
            rowwin &= boardData[i*board_dim + j] == mark;
            colwin &= boardData[j*board_dim + i] == mark;
        }
        if (colwin || rowwin)
            return true;
    }
    //check for across patterns, one space between each marked space for solution...
    if (boardData[space3] == boardData[space5] && boardData[space5] == boardData[space7])
        return true;
    if (boardData[space1] == boardData[space5] && boardData[space5] == boardData[space9])
        return true;

    return false;
}

bool Data::checkCatsGame(const int &turnCnt) const{
    if (turnCnt < MAX_TURNS)
        return false;
    else
        return true;
}

void Data::gameReset() {
    boardData = "123456789";
}

Game.h

#ifndef GAME
#define GAME

#include "Data.h"
#include "Screen.h"

class Game
{
    Data board;
    Screen view;
public:

     Game() = default;
    ~Game() = default;
    void turn(const char&);
    void run();
    bool getReplayInput();
};

#endif // !GAME

Game.cpp

#include "Game.h"
#include "Constants.h"
#include <iostream>

void Game::turn(const char &mark) {
    bool inputCheck = false;
    size_t posChoice;
    while (!inputCheck) {
        //checks data type
        if (std::cin >> posChoice) {
            if (posChoice < 10 && posChoice > 0) {
                char spaceCheck = board.boardSpaceValue(posChoice);
                //checks if space has been marked
                if (spaceCheck != 'X' && spaceCheck != 'O') {
                    board.markBoard(posChoice, mark);
                    inputCheck = true;
                }
                else {
                    view.errorMsg(errBoard); //Space has already been marked
                }
            }
            else {
                view.errorMsg(errRecognize); //Space not within board range
            }
        }
        else {
            view.errorMsg(errType); //incorrect data type
            std::cin.clear();
            std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        }
    }
}

bool Game::getReplayInput() {
    bool inputCheck = false;
    char yesOrNo;
    while (!inputCheck) {
        if (std::cin >> yesOrNo) {
            yesOrNo = toupper(yesOrNo);
            if (yesOrNo == 'Y') {
                return true;
            }
            else if (yesOrNo == 'N') {
                return false;
            }
            else {
                view.errorMsg(errRecognize);
            }
        }
        else {
            view.errorMsg(errType);
            std::cin.clear();
            std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        }
    }

}

void Game::run() {
    bool gameWin = false, tie = false, playAgain = false;
    int turnCnt = 0;
    //setup
    view.welcomeMsg();
    view.draw(board.printBoard());
    //gameplay
    while (!gameWin && !tie) {
        turnCnt++;
        char playerMark = ((turnCnt % 2) ? board.player1Mark() : board.player2Mark());
        view.signalUserInput((playerMark == 'X') ? 1 : 2);
        turn(playerMark);
        gameWin = board.checkWin(playerMark);
        tie = board.checkCatsGame(turnCnt);
        view.draw(board.printBoard());
    }
    //signal winner
    if (gameWin) {
        view.gameWinMsg(turnCnt);
    }
    //signal tie game
    else {
        view.gameTieMsg();
    }
    //check if user wishes to replay
    playAgain = getReplayInput();
    if (playAgain) {
        board.gameReset();
        run();
    }
}

Screen.h

#ifndef SCREEN
#define SCREEN

#include <string>

class Screen
{
public:
    Screen() = default;
    ~Screen() = default;
    void errorMsg(const int&) const;
    void signalUserInput(const int&) const;
    void welcomeMsg() const;
    void draw(const std::string &) const;
    void gameWinMsg(const size_t&) const;
    void gameTieMsg() const;
};

#endif

Screen.cpp

#include "Screen.h"
#include "Constants.h"
#include <iostream>

void Screen::errorMsg(const int& errNumber) const{
    if (errNumber == errRecognize) {
        std::cout << "Input not recognized, try again:\n";
    }
    if (errNumber == errType) {
        std::cout << "Incorrect data type, try again:\n";
    }
    if (errNumber == errBoard) {
        std::cout << "Board space has already been marked, try agian:\n";
    }
}

void Screen::signalUserInput(const int &player) const{
    std::cout << "Player" << player <<" please select a space to mark: ";
}

void Screen::welcomeMsg() const {
    std::cout << "Welcome to tic tac toe, player1 will be 'X' and player2 will be 'O'\n";
}

void Screen::gameWinMsg(const size_t& turnCnt) const{
    if (turnCnt % 2) {
        std::cout << "Player 1 wins the game!\nPlay again? Y/N: ";
    }
    else
        std::cout << "Player 2 wins the game!\nPlay again? Y/N: ";
}

void Screen::gameTieMsg() const{
    std::cout << "Tie!\nGame over!\nPlay again? Y/N: ";
}

void Screen::draw(const std::string &board) const{
    std::cout << " " << board[space1] << " " << "|" << " " << board[space2] << " " << "|" << " " << board[space3] << "\n"
        << "___|___|___\n"
         << " " << board[space4] << " " << "|" << " " << board[space5] << " " << "|" << " " << board[space6] << "\n"
        << "___|___|___\n"
        << " " << board[space7] << " " << "|" << " " << board[space8] << " " << "|" << " " << board[space9] << "\n\n";
}

Source.cpp

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

void pause() {
    std::string pause;
    std::cout << "Press any key followed by enter to continue...";
    std::cin >> pause;
}

int main() {
    Game game;
    game.run();
    pause();
}
\$\endgroup\$

2 Answers 2

6
\$\begingroup\$

This certainly seems like a step in the right direction. I have some suggestions to take it even further.

Use The Right Structure

This code is more complicated than it needs to be and one reason why is that you're not using the right structures for your data. A tic-tac-toe board is not a 1 dimensional stream of characters. It's a 2 dimensional board, and your data should reflect that.

But before you even get there, you're also not using a reasonable type for your board. You shouldn't store the board as a std::string. A string is composed of chars and there are 256 possible char values where you only need 3 specific values - empty, x, and o. When you have a few specific types you need, that's a good place to use an enumerated type. I recommend something like this:

enum boardCell : char {
    empty = ' ',
    x = 'x',
    o = 'o'
};

Once you have the right type you can create a 2D array of boardCells:

boardCell boardState[ kMaxRows ][ kMaxCols ] = {
    { empty, empty, empty },
    { empty, empty, empty },
    { empty, empty, empty }
};

Here I'm defining kMaxRows and kMaxCols as constants that represent the number of rows and columns on your board. Something like this:

const size_t kMaxRows = 3;
const size_t kMaxCols = 3;

If you do want to allow larger boards in the future, you can allocate the board dynamically and the maximum number of rows and columns would no longer be constant.

Once you have this setup, you no longer need to reference specific squares or use magic numbers. You can draw the board like this:

for (int row = 0; row < kMaxRows; row++)
{
    std:cout << "__|__|__\n";
    for (int col = 0; col < kMaxCols; col++)
    {
        std::cout << " " << boardState [ row ][ col ];
        if (col < kMaxCols - 1)
        {
            std::cout << "|";
        }
    }
    std::cout << "\n";
}

You can also check whether anyone has won the game by looping over rows, columns, and diagonals and seeing if they match.

Naming

You have a class named Data. Almost all classes hold data. Data is a generic word that basically means "stuff". This class appears to manage the current state of the game. So why not call it GameState?

The name Game is much better, but not very specific. Perhaps a name like TicTacToeGame would be better? That way we know it's not Space Invaders or Doom or Poker.

Likewise, I don't find the name Screen to be very descriptive. It doesn't represent a computer screen. And it doesn't screen data. A better name might be something like GameView, since you actually call it a view in the Game class.

Who Does What?

I see a few places where you make one object decide what another object should do. For example, in Game::run(), you have the Game object tell the view which user it should signal for input, but 2 lines later, you simply tell the board the check whether the current player has run:

    view.signalUserInput((playerMark == 'X') ? 1 : 2); // <- Game object decides which user to signal
    turn(playerMark);
    gameWin = board.checkWin(playerMark); // <- Board decides based on playerMark

I would expect these to be consistent. I would just make signalUserInput have the logic to figure out whether player 1 is X or O, or just pass in a boardCell instead of the thing that is displayed to represent a board cell.

Likewise, I would allow the view to determine what message to output based on the gameWin variable.

\$\endgroup\$
1
  • \$\begingroup\$ You missed the words "at least" in front of "256 possible char values. Pedantry aside, this is a great answer. \$\endgroup\$ Commented Nov 6, 2017 at 9:48
3
\$\begingroup\$

The Data::checkWin function does not scale to other board sizes. And I don't think tic-tac-toe will ever be played on other board sizes. Therefore the code can be made even simpler:

static bool is_triplet(const std::string &boardData, char mark, size_t a, size_t b, size_t c) {
    return boardData[a] == mark
        && boardData[b] == mark
        && boardData[c] == mark;
}

bool Data::checkWin(const char &mark) const {
    return is_triplet(boardData, mark, 0, 1, 2)
        || is_triplet(boardData, mark, 3, 4, 5)
        || is_triplet(boardData, mark, 6, 7, 8)

        || is_triplet(boardData, mark, 0, 3, 6)
        || is_triplet(boardData, mark, 1, 4, 7)
        || is_triplet(boardData, mark, 2, 5, 8)

        || is_triplet(boardData, mark, 0, 4, 8)
        || is_triplet(boardData, mark, 2, 4, 6);
}

In your current code, it seems that an empty diagonal also counts as won since you don't check whether boardData[5] == mark.

If you don't like these "magic" numbers, you have to eliminate them all. Even the 3 and 5 and 7, and the implicit 3 (because 3 5 7 are 3 numbers).

\$\endgroup\$

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.