HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Tic Tac Toe C++ follow up

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
tacticfollowtoe

Problem

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 

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 &pl

Solution

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.

Code Snippets

enum boardCell : char {
    empty = ' ',
    x = 'x',
    o = 'o'
};
boardCell boardState[ kMaxRows ][ kMaxCols ] = {
    { empty, empty, empty },
    { empty, empty, empty },
    { empty, empty, empty }
};
const size_t kMaxRows = 3;
const size_t kMaxCols = 3;
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";
}
view.signalUserInput((playerMark == 'X') ? 1 : 2); // <- Game object decides which user to signal
    turn(playerMark);
    gameWin = board.checkWin(playerMark); // <- Board decides based on playerMark

Context

StackExchange Code Review Q#126292, answer score: 6

Revisions (0)

No revisions yet.