3
\$\begingroup\$

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 << "keyInput " << keyInput << std::endl;
    switch(keyInput)
    {

        case (1):
            if (board[2][0] != '*')
            {
                std::cout << "Already take!\n";
                obj->Graphic.iClearScreen();
                obj->Graphic.iPrintBoard(this);
                std::cout << "Already take!\n";
                obj->playerMove(this);
            }
            else
                board[2][0] = obj->getUserID();
                moves++;
            //std::cout << "first one here eh mate! " << std::endl;
            //std::cout << this->board[2][0] << std::endl;
            break;
        case (2):
            if (board[2][1] != '*')
            {
                std::cout << "Already take!\n";
                obj->Graphic.iClearScreen();
                obj->Graphic.iPrintBoard(this);
                std::cout << "Already take!\n";
                obj->playerMove(this);
            }
            else
                board[2][1] = obj->getUserID();
                moves++;
            break;
        case (3):
            if (board[2][2] != '*')
            {
                std::cout << "Already take!\n";
                obj->Graphic.iClearScreen();
                obj->Graphic.iPrintBoard(this);
                std::cout << "Already take!\n";
                obj->playerMove(this);
            }
            else
                board[2][2] = obj->getUserID();
                moves++;
            break;
        case (4):
            if (board[1][0] != '*')
            {
                std::cout << "Already take!\n";
                obj->Graphic.iClearScreen();
                obj->Graphic.iPrintBoard(this);
                std::cout << "Already take!\n";
                obj->playerMove(this);
            }
            else
                board[1][0] = obj->getUserID();
                moves++;
            break;
        case (5):
            if (board[1][1] != '*')
            {
                std::cout << "Already take!\n";
                obj->Graphic.iClearScreen();
                obj->Graphic.iPrintBoard(this);
                std::cout << "Already take!\n";
                obj->playerMove(this);
            }
            else
                board[1][1] = obj->getUserID();
                moves++;
            break;           
        case (6):
            if (board[1][2] != '*')
            {
                std::cout << "Already take!\n";
                obj->Graphic.iClearScreen();
                obj->Graphic.iPrintBoard(this);
                std::cout << "Already take!\n";
                obj->playerMove(this);
            }
            else
                board[1][2] = obj->getUserID();
                moves++;
            break;  
        case (7):
            if (board[0][0] != '*')
            {
                std::cout << "Already take!\n";
                obj->Graphic.iClearScreen();
                obj->Graphic.iPrintBoard(this);
                std::cout << "Already take!\n";
                obj->playerMove(this);
            }
            else
                board[0][0] = obj->getUserID();
                moves++;
            break;
        case (8):
            if (board[0][1] != '*')
            {
                std::cout << "Already take!\n";
                obj->Graphic.iClearScreen();
                obj->Graphic.iPrintBoard(this);
                std::cout << "Already take!\n";
                obj->playerMove(this);
            }
            else
                board[0][1] = obj->getUserID();
                moves++;
            break;
        case (9):
            if (board[0][2] != '*')
            {
                std::cout << "Already take!\n";
                obj->Graphic.iClearScreen();
                obj->Graphic.iPrintBoard(this);
                std::cout << "Already take!\n";
                obj->playerMove(this);
            }
            else
                board[0][2] = obj->getUserID();
                moves++;
            break;
    }
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Please either include the code to be reviewed either in its entirety, or the part you're most interested in having reviewed. \$\endgroup\$ Apr 11, 2016 at 21:49
  • \$\begingroup\$ Nice! Edit my post to say whatever you feel like it should say. \$\endgroup\$
    – justthom8
    Apr 11, 2016 at 22:51

1 Answer 1

2
\$\begingroup\$

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 << "Already taken!\n";
    obj->Graphic.iClearScreen();
    obj->Graphic.iPrintBoard(this);
    std::cout << "Already taken!\n";
    obj->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).

\$\endgroup\$
6
  • \$\begingroup\$ I don't understand your first bit of code. Can you explain it a little bit. It may be my inexperience, but I don't understand having what looks like an array, positions[10] with a board_position type. \$\endgroup\$
    – justthom8
    Apr 11, 2016 at 22:46
  • \$\begingroup\$ @justthom8: You don't have code here to tell me board's type, so I was using board_position as an alias for that unknown type. Let's say your board is something like char array[3][3];. In this case, board_position would be char *. \$\endgroup\$ Apr 11, 2016 at 22:57
  • \$\begingroup\$ Ah, I see. What about positions[10]? That's where I got confused. My compiler complained that I couldn't use that kind of initialization unless I enabled C++11 flag. Sorry for being a noob, but it's hard to google very specific questions like this. \$\endgroup\$
    – justthom8
    Apr 11, 2016 at 23:22
  • \$\begingroup\$ There is no positions[10]. When you define an array like int array[10], it means that the legal subscripts are array[0] through array[9]; (I defined it as an array of 10 because you were using 1 through 9, and it seemed easiest to just use that, and basically ignore positions[0]). \$\endgroup\$ Apr 11, 2016 at 23:25
  • \$\begingroup\$ Ohhh, so you mean to put an equal sign there. That makes far more sense than some kind of class definition, or new C++ feature I don't know. \$\endgroup\$
    – justthom8
    Apr 12, 2016 at 0:08

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.