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

TicTacToe game with large switch statements

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

Problem

This 5 classes: CBoard, CInput, CPlayer, Console, Graphics, and the main file, Test, which is actually main.cpp (but I named it poorly). Specifically, I use a switch to update the board, which is a 2D char array.

Is there a better way to update the board besides a large switch case? I have yet to think of a way to hold the two array indices of the board. Please give me some constructive criticism on my console TicTacToe game? It would be difficult to post all 10 files though, so I've uploaded it to GitHub (but only review the code in this post).

```
void CBoard::iCustomUpdateBoard(CPlayer obj, CInput UserInput)
{
unsigned keyInput = UserInput->getSingleNumPadKey();
//std::cout Graphic.iClearScreen();
obj->Graphic.iPrintBoard(this);
std::cout playerMove(this);
}
else
board[2][0] = obj->getUserID();
moves++;
//std::cout board[2][0] Graphic.iClearScreen();
obj->Graphic.iPrintBoard(this);
std::cout playerMove(this);
}
else
board[2][1] = obj->getUserID();
moves++;
break;
case (3):
if (board[2][2] != '*')
{
std::cout Graphic.iClearScreen();
obj->Graphic.iPrintBoard(this);
std::cout playerMove(this);
}
else
board[2][2] = obj->getUserID();
moves++;
break;
case (4):
if (board[1][0] != '*')
{
std::cout Graphic.iClearScreen();
obj->Graphic.iPrintBoard(this);
std::cout playerMove(this);
}
else
board[1][0] = obj->getUserID();
moves++;
break;
case (5):
if (board[1][1] != '*')
{
std::cout Graphic.iClearScreen();

Solution

The code you've posted is highly repetitive. You have 9 repetitions of nearly identical code, with a couple of instances of board[x][y], where x and y change, but everything else is identical.

I'd rather see (for just one possibility) a small array defining the mapping from input number (1..9) to 2D board position:

board_position positions[10] { 
    nullptr,
    &board[2][0],
    &board[2][1],
    &board[2][2],
    // ...
    &board[0][2]
};


Using that, the rest of the code collapses down to one piece of code for everything:

auto pos = positions[keyInput];

if (*pos != '*') {
    std::cout Graphic.iClearScreen();
    obj->Graphic.iPrintBoard(this);
    std::cout playerMove(this);
}
else {
    *pos = obj->getUserID();
    moves++;
}


Although I haven't shown it here, it would also be possible to create variables named x and y (just for example) and compute them based on the user's input. Either way, you end up replacing 9 nearly identical pieces of code with 1.

Also note that I've added braces around statements controlled by the else. As it was, your indentation indicated that you probably wanted the control flow shown above, but since you didn't have braces, the moves++ was not under control of the if/else at all (another problem that clearly needs fixing).

Code Snippets

board_position positions[10] { 
    nullptr,
    &board[2][0],
    &board[2][1],
    &board[2][2],
    // ...
    &board[0][2]
};
auto pos = positions[keyInput];

if (*pos != '*') {
    std::cout << "Already taken!\n";
    obj->Graphic.iClearScreen();
    obj->Graphic.iPrintBoard(this);
    std::cout << "Already taken!\n";
    obj->playerMove(this);
}
else {
    *pos = obj->getUserID();
    moves++;
}

Context

StackExchange Code Review Q#125423, answer score: 2

Revisions (0)

No revisions yet.