C++

Forum

 
OOP TicTacToe Critique

CpluspProgrammer (18)
Hey there programmers, I have written a oop version of Tic Tac Toe. If you could critique my code and let me know if there is a more efficient and better way of programming something in my code. Thank you.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
 
  #include <iostream>

using namespace std;


class TicTacToe {
public:
	TicTacToe() { }
	~TicTacToe() {}
	int playerWins;
	bool isDraw;
	void draw_board(char board[])
	{
		cout << board[0] << " " << board[1] << " " << board[2] << endl;
		cout << "-+-+-" << endl;
		cout << board[3] << " " << board[4] << " " << board[5] << endl;
		cout << "-+-+-" << endl;
		cout << board[6] << " " << board[7] << " " << board[8] << endl;

	}
	void clearScreen(){
		for (int i = 0; i < 50; i++)
			cout << endl;
	}
	bool isGameOver(char board[])
	{
		if (board[0] == 'X' && board[1] == 'X' && board[2] == 'X' || board[0] == 'X' && board[3] == 'X' && board[6] == 'X' || board[3] == 'X' && board[4] == 'X' && board[5] == 'X'
			|| board[6] == 'X' && board[7] == 'X' && board[8] == 'X' || board[1] == 'X' && board[4] == 'X' && board[7] == 'X' || board[2] == 'X' && board[5] == 'X' && board[8] == 'X'
			|| board[0] == 'X' && board[4] == 'X' && board[8] == 'X' || board[2] == 'X' && board[4] == 'X' && board[6] == 'X') {
			this->playerWins = 1;
			return true;
		}
		else if (board[0] == 'O' && board[1] == 'O' && board[2] == 'O' || board[0] == 'O' && board[3] == 'O' && board[6] == 'O' || board[3] == 'O' && board[4] == 'X' && board[5] == 'O'
			|| board[6] == 'O' && board[7] == 'O' && board[8] == 'O' || board[1] == 'O' && board[4] == 'O' && board[7] == 'O' || board[2] == 'O' && board[5] == 'X' && board[8] == 'O'
			|| board[0] == 'O' && board[4] == 'O' && board[8] == 'O' || board[2] == 'O' && board[4] == 'O' && board[6] == 'O') {
			this->playerWins = 2;
			return true;
		}
		else {
			isDraw = true;
		}
		return false;
	}

};

int main() {
	TicTacToe classhandler;
	int playerTurn = 1;
	char playerMark, choice;
	char board[] = { '1', '2', '3', '4', '5', '6', '7', '8', '9' }; 
	do {
		classhandler.clearScreen();
		classhandler.draw_board(board);
		if (playerTurn == 1)
		{
			playerMark = 'X';
		}
		else if (playerTurn == 2)
		{
			playerMark = 'O';
		}
		cout << "Player " << playerTurn << ": ";
		cin >> choice;
		switch (choice)
		{
		case '1':
			if (playerTurn == 1)
			{
				board[0] = playerMark;
				playerTurn = 2;
			}
			else if (playerTurn == 2)
			{
				board[0] = playerMark;
				playerTurn = 1;
			}
			break;
		case '2':
			if (playerTurn == 1)
			{
				board[1] = playerMark;
				playerTurn = 2;
			}
			else if (playerTurn == 2)
			{
				board[1] = playerMark;
				playerTurn = 1;
			}
			break;
		case '3':
			if (playerTurn == 1)
			{
				board[2] = playerMark;
				playerTurn = 2;
			}
			else if (playerTurn == 2)
			{
				board[2] = playerMark;
				playerTurn = 1;
			}
			break;
		case '4':
			if (playerTurn == 1)
			{
				board[3] = playerMark;
				playerTurn = 2;
			}
			else if (playerTurn == 2)
			{
				board[3] = playerMark;
				playerTurn = 1;
			}
			break;
		case '5':
			if (playerTurn == 1)
			{
				board[4] = playerMark;
				playerTurn = 2;
			}
			else if (playerTurn == 2)
			{
				board[4] = playerMark;
				playerTurn = 1;
			}
			break;
		case '6':
			if (playerTurn == 1)
			{
				board[5] = playerMark;
				playerTurn = 2;
			}
			else if (playerTurn == 2)
			{
				board[5] = playerMark;
				playerTurn = 1;
			}
			break;
		case '7':
			if (playerTurn == 1)
			{
				board[6] = playerMark;
				playerTurn = 2;
			}
			else if (playerTurn == 2)
			{
				board[6] = playerMark;
				playerTurn = 1;
			}
			break;
		case '8':
			if (playerTurn == 1)
			{
				board[7] = playerMark;
				playerTurn = 2;
			}
			else if (playerTurn == 2)
			{
				board[7] = playerMark;
				playerTurn = 1;
			}
			break;
		case '9':
			if (playerTurn == 1)
			{
				board[8] = playerMark;
				playerTurn = 2;
			}
			else if (playerTurn == 2)
			{
				board[8] = playerMark;
				playerTurn = 1;
			}
			break;
		default:
			break;
		}
	} while (!classhandler.isGameOver(board));
	classhandler.clearScreen();
	classhandler.draw_board(board);
	if (classhandler.playerWins = 1)
	{
		cout << "Game Over! Player One Wins!" << endl;
	}
	else if (classhandler.playerWins = 2)
	{
		cout << "Game Over! Player Two Wins!" << endl;
	}
	else {
		cout << "Game Over! Its a draw!" << endl;
	}
	system("pause");
	return 0;
}
coder777 (8444)
The switch on line 65 (and the code repetition) can completely eliminated: 
1
2
3
4
5
 
const int idx = choice - '1';
if((idx < 0) or (idx > 8))
  cout << "Invalid move";
else
  ...


You can simplify the handling of playerTurn and playerMark when using an array.

Note the error on line 181 and 185.
Last edited on
MiiNiPaa (8886)
Also your TicTacToe class is stateless. There is no real need to have an instance of it or to define constructrors and sestructors: all members would work better if made static. And in C++ it is often suggested that instead of making a class with all static membrs, it is better to just place them inside of namespace. So your class is really redundand here.

To make it OOP you have to delegate responsibility to manage board to class itself instead of doing it manually in main. Ideally your main function should look like:
1
2
3
4
5
 
TicTacToe game;
while(game.state() == game::IN_PROGRESS) {
    game.display();
    game.next_turn();
}
andywestken (4094)
Is even the game loop needed?

1
2
3
4
5
6
7
 
int main() {
    TicTacToe game;

    game.play(); // run game loop

    return 0;
}


or even

1
2
3
4
5
 
int main() {
    TicTacToe game; // automatically run game loop

    return 0;
}


A game loop could be useful, however, if you wanted to decouple the input/output code.

Meanwhile...

Both draw_board() and isGameOver() could be simplified with the use of loops (esp. if isGameOver() looks for rows and cols where all values are equal, rather than checking for specific values.)

Andy
Last edited on
MiiNiPaa (8886)
I would not place game loop in constructor. Bad idea.

And loop is still going to be somewhere. It is only matter of for what extent your class manages game. For example we cam wrap my TicTacToe class with some other Game class which would just move that loop out of main inside play() ethod of that class. Again, depending on your needs and architecture of application.
coder777 (8444)
There is another code repetition in bool isGameOver(char board[]). You can avoid it like so:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
 
	bool isGameOver(const char board[], const char mark)
	{
		return (board[0] == mark && board[1] == mark && board[2] == mark || board[0] == mark && board[3] == mark && board[6] == mark || board[3] == mark && board[4] == mark && board[5] == mark
			|| board[6] == mark && board[7] == mark && board[8] == mark || board[1] == mark && board[4] == mark && board[7] == mark || board[2] == mark && board[5] == mark && board[8] == mark
			|| board[0] == mark && board[4] == mark && board[8] == mark || board[2] == mark && board[4] == mark && board[6] == mark);
	}
	bool isGameOver(const char board[])
	{
		if (isGameOver(board, 'X')) {
			this->playerWins = 1;
			return true;
		}
		if (isGameOver(board, 'O')) {
			this->playerWins = 2;
			return true;
		}
		else {
			isDraw = true;
		}
		return false;
	}
andywestken (4094)
I would not place game loop in constructor. Bad idea.

In a trivial game I don't see this to be an actual problem. I have come across the opinion that a truly object oriented C++ program's entrypoint should be the constructor of the "application" object. But I would probably construct the instance and then call a play() method.

As I already said, I only see the point of an external loop if the game object needs to interact with the system (i.e. there is i/p and o/p).

And TicTacToe is a Game...

from : https://en.wikipedia.org/wiki/Tic-tac-toe
Tic-tac-toe (or Noughts and crosses, Xs and Os) is a paper-and-pencil computer game for two players, X and O, who take turns marking the spaces in a 3×3 grid.

so I would not expect it to be wrapped by a game object.

It is only matter of for what extent your class manages game.

From an OO point of view, the class is the game, it does not manage it.

Andy
Last edited on
andywestken (4094)
Rather than checking if the game is over, you could search for a winner. The function lookk for a line of chars which are the same (so the assumption is their init to different chars as above.)

The instead of setting the int playerWins to 1 or 2 could make it a char and set it to 'X' or '0'.

Andy

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
 
    char findWinner(const char board[])
    {
        for(int row = 0; row < 3; ++row)
        {
            if(    board[3 * row + 0] == board[3 * row + 1]
                && board[3 * row + 1] == board[3 * row + 2] )
                return board[3 * row + 0];
        }

        for(int col = 0; col < 3; ++col)
        {
            if(    board[col + 0] == board[col + 3]
                && board[col + 3] == board[col + 6] )
                return board[col + 0];
        }

        // check diaganols
        if (     ((board[4] == board[0]) && (board[4] == board[8]))
              || ((board[4] == board[2]) && (board[4] == board[6])) )
        {
              return board[4];
        }

        return 'N'; // for no one!
    }
CpluspProgrammer (18)
WoW thanks for the replies guys, a lot of good information here for me to improve my code. Thanks I really appreciate it.
Topic archived. No new replies allowed.