patterncppMinor
Tic Tac Toe with classes
Viewed 0 times
withtoetictacclasses
Problem
I have created Tic Tac Toe in c++ using classes. Is this efficient code?
And GNU GCC compiler gives these warnings:
This is the main function calling all of the functions:
And GNU GCC compiler gives these warnings:
Warning: control reaches end of non-void function [-Wreturn-type]
Warning: extended initializer lists only available with -std=c++11 or -std=gnu++11 [enabled by default]
warning: non-static data member initializers only available with -std=c++11 or -std=gnu++11 [enabled by default]
TicTacToe.h:#ifndef TICTACTOE_H_INCLUDED
#define TICTACTOE_H_INCLUDED
class TicTacToe
{
public:
TicTacToe();
~TicTacToe();
void welcome();
const void getInput();
private:
char blockEntries[9] = {'1', '2', '3', '4', '5', '6', '7', '8', '9'};
int checkStatus() const;
int turns = 10;
void printBoard() const;
const void resetBoard();
};
#endif // TICTACTOE_H_INCLUDEDTicTacToe.cpp:#include // Standard Input output stream
#include "TicTacToe.h" // Game class Header file
#include
#include
TicTacToe::TicTacToe() // Default Constructor
{
}
void TicTacToe::welcome() // Shows Welcome message
{
std::cout > input; // input array numbers
if((input >= 1 && input Player" 1);
if(turns == 1)
{
std::cout Player1 and Player2 Tie X Player2 ---> O" 1)
)
{
return 1;
}
}
const void TicTacToe::resetBoard()
{
int values = 49; // '1' has value 49
for(int i = 0; i <=9; i++ )
{
blockEntries[i] = values;
values += 1;
}
}This is the main function calling all of the functions:
#include
#include "TicTacToe.h"
int main()
{
char restart;
TicTacToe game;
game.welcome();
game.getInput();
std::cout > restart;
if (restart == 'y')
{
std::cout << "\n\n\n";
game.welcome();
game.getInput();
}
else if (restart == 'n')
return 0;
}Solution
-
In general you should keep separate the logic of your application from the user interface. So your TicTacToe object should represent the state of the game (player turn, content of the board) and heave methods to play a move (and validate it), and inspect the status. In this simple program I would prefer to keep the input/output code inside main or in some helper function.
-
Since your constructor and destructor do nothing, don't declare them. You spare to write more code, you don't need to comment them being 'default' and the user looking at the *.h file has something less to care about.
-
Your
The same is true for the getInput function which should have both input and output streams as parameters. Anyway, as already said both function should not be part of the class implementing the logic of the game... In particular
-
The
In general you should keep separate the logic of your application from the user interface. So your TicTacToe object should represent the state of the game (player turn, content of the board) and heave methods to play a move (and validate it), and inspect the status. In this simple program I would prefer to keep the input/output code inside main or in some helper function.
-
Since your constructor and destructor do nothing, don't declare them. You spare to write more code, you don't need to comment them being 'default' and the user looking at the *.h file has something less to care about.
-
Your
welcome function should accept the output stream as an argument. This shortens the function itself (you can use out instead of std::cout) and make it more general. In fact it is good to use std::cout only in your main function.The same is true for the getInput function which should have both input and output streams as parameters. Anyway, as already said both function should not be part of the class implementing the logic of the game... In particular
getInput is the main loop of the program, which could well be in a function outside the TicTacToe class.-
The
printBoard function is actually good to have also if TicTacToe only represents the logic of the game, because it can be used to have a textual representation of the object. However you should implement it overloading the friend operator
-
Even if your board is very small (3x3) it is better to use loops instead of repeating the code when you iterate over the rows. This makes the code a little bit shorter but is much more maintainable when you want to make modifications. This is true in both printBoard and checkStatus.
-
the name of the variable turns is misleading. If you represent the number of turns you expect it to increase. Instead you are maybe representing the number of empty cells. It is very difficult to understand if the check of the do...while i.e. turns>1 is correct or is off by one.
-
the variable player should only have values 1 and 0. So you should make the modulus check after increasing the value, not at the beginning of the following loop. This will preserve the invariant of the loop.
-
filling the board with the numbers representing possible moves is something related to the output functions not the logic of the game. Better to have only three possible values for EMPTY, PLAYER1, PLAYER2 representing the content of the cells. You are using the fact that empty cells are different each other in the checkStatus function. This is very smart, but not good if you want the code to be easy to understand and easy to maintain. If someone would like to implement a graphic interface he will not need to put numbers in the cells, but will run the risk to break the code if it tries to modify it.
-
conio.h is a non standard header. The function checkStatus should return 0` (this is causing the warning).Context
StackExchange Code Review Q#61006, answer score: 6
Revisions (0)
No revisions yet.