Avoid using system()
for trivial tasks
Before resorting to calling external programs for simple tasks, try to find out how to do them in C++ itself. See https://stackoverflow.com/questions/37774983/clearing-the-screen-by-printing-a-character/37778152 for how to clear the screen by printing ANSI escape codes from within C++. There are also escape codes for setting the foreground and background colours.
Apart from not being very portable, system()
has a huge overhead.
Prefer '\n'
over std::endl
[std::endl][1]
is equivalent to '\n'
plus a flush of the output buffer. This flushing is adding unnecessary overhead. So just end strings with \n
, it's faster and less typing.
Avoid forward declarations
You are forward-declaring the functions mainMenu()
, play()
, music()
and colour()
. If you move the implementation of these functions to above main()
, you don't need the forward declarations anymore. This avoids having to repeat the function prototypes. Not having to repeat yourself means less chance of adding errors.
Use switch
where appropriate
In mainMenu()
, after reading a key, you use a sequence of if
-statements to do things depending on which key was pressed. Instead of a whole list of if
-else
statements, this is normally a job for switch
:
switch (userChoice) {
case '1':
play();
break;
case '2':
std::cout << "Instructions: ...\n";
break;
...
csae '7':
std::cout << "Thank you for playing, goodbye\n";
return;
default:
std::cout << "Error - Please choose again\n";
break;
}
Make functions and global variables static
where appropriate
Functions and global variables that are only used by functions inside the same source file should be made static
. This tells the compiler nothing else needs these functions, and then it can more aggressively optimize the code.
Avoid repetition
Whenever you are repeating some lines of codes multiple times with only small variations, you should immediately stop and try to turn it into for
-loop or find some other way to avoid this repetition. For example, in Input()
, instead of writing a test for each possible position, and then manually setting the right matrix element, you could have written a for
-loop:
for (int i = 1; i <= 9; i++)
if (a == i && matrix[i / 3][i % 3] == '0' + i)
matrix[i / 3][i % 3] = player;
However, in this case you don't need a for
-loop at all, and can just write:
if (a >= 1 && a <= 9)
if (matrix[a / 3][a % 3] == '0' + a)
matrix[a / 3][a % 3] = player;
Also, in Win()
you can get rid of many lines of code by using loops.
Create a class to represent the board
The next improvement would be to make a class that represents the board, and have member functions in that class to make moves, check for winning conditions, and so on. This class should not read user input or write menus to the screen, however you can add a function to just print the state of the board. An example of what the class might look like:
class TicTacToeBoard {
char matrix[3][3] = {'1', '2', ...};
char player = 'X';
public:
void togglePlayer();
char getCurrentPlayer();
char checkWinner();
bool placeMarker(int pos);
void print(std::ostream &output);
void reset();
};
Have these member functions do error checking where necessary, for example have placeMarker()
check that the given position is a valid number and that no other marker has been placed at that position yet, and return true
if the marker was placed succesfully, false
otherwise. Then for example, in Input()
you can write:
while (true) {
cout << "It's " << board.getCurrentPlayer() << "'s turn. Enter a number: ";
cin >> a;
if (board.placeMarker(a))
return;
else
cout << "Invalid input, please try again.\n";
}
See how this makes each function smaller and more readable.
Use enum
s for players
Using a raw char
for representing the current player and for each position of the board is of course possible, but it has the issue that a char
can have many possible values, most of which are invalid. Also, you might later want to change the way you print the board (maybe with nicer Unicode characters for representing circles and crosses), and then it will be a lot of work to change your code. It is better to use an enum
, or even better an enum class
, to clearly define the possible values:
enum class Player {
NONE,
X,
O,
};
Then, make your board and current player variables use this:
Player matrix[3][3] = {}; // initializes everything to Player::NONE
Player player = Player::X;
Then, for example when drawing the board, check the enum value of each position, and then convert that into the actual character you want to print:
for (int row = 0; row < 3; row++) {
for (int col = 0; col < 3; col++) {
switch (matrix[row][col]) {
case Player::NONE:
std::cout << row * 3 + col + 1 << " ";
break;
case Player::X:
std::cout << "X ";
break;
case Player::O:
std::cout << "O ";
break;
}
}
std::cout << "\n";
}
Of course, this is a bit more typing here, but you could create a function takes a Player
value and returns a char
or std::string
representation of it, and call that function in Input()
.
The advantage of this is that it's much harder to accidentily have an invalid value in matrix[][]
, and when used in a switch
-statement, the compiler will actually warn you if you didn't handle all the possible values of the enum
.