patterncppMinor
Tic Tac Toe C++ follow up
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
Constants.h
Data.h
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
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
#endifData.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;
};
#endifData.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
Once you have the right type you can create a 2D array of
Here I'm defining
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:
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
The name
Likewise, I don't find the name
Who Does What?
I see a few places where you make one object decide what another object should do. For example, in
I would expect these to be consistent. I would just make
Likewise, I would allow the view to determine what message to output based on the
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 playerMarkI 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 playerMarkContext
StackExchange Code Review Q#126292, answer score: 6
Revisions (0)
No revisions yet.