Noughts and Crosses Game Feedback

Question

Hi i have created a game of noughts and crosses in c++ I am new to this so the code is a bit messy, but i would love to get any feedback from you lot about the actual game and not the code for now.

main functions that i have added is playable music from the beginning which can be turned off in the menu and also the option to change the colours of the background and the text from the menu.

#include <iostream>
#include <conio.h>
#include <stdio.h>
#include <stdlib.h>
#include <Windows.h>
#include <MMSystem.h>


using namespace std;
char matrix[3][3] = { '1', '2', '3', '4', '5', '6', '7', '8', '9' };
int rear;
char player = 'X';
void mainMenu();
void play();
void music();
void colour();

//Strings for the Player Names to be Chosen//
string charName = "Player1";
string charName2 = "Player 2";

//Noughts and Crosses Home Screen//
int main()
{

    PlaySound(TEXT("Tetris.wav"), NULL, SND_ASYNC);


    char userChoice = 'o';

    cout << "\n\n\n\n\n\n\n\n\n\n\n                                        WELCOME TO NOUGHTS AND CROSSES" << endl
        << "\n\n                                           Press ENTER to continue  ";
    cin.ignore();

       mainMenu();


    return 0;
}

//Main Menu With Options//
void mainMenu()
{
    system("CLS");

    char userChoice = 'o';
    do
    {
        //Main Menu//

        cout << "\n\n\n          Welcome to Noughts and Crosses" << endl
            << "\n\n          1. Play Game " << endl            
            << "          2. How to Play" << endl
            << "          3. Music"<< endl
            << "          4. Change Colours" << endl
            << "          5. Game Details" << endl
            << "          6. Credits" << endl
            << "          7. Exit" << endl;
        cout << "\n          Please make a choice:  ";

        cin >> userChoice;
        system("CLS");
        if (userChoice == '1')
            play();
        else if (userChoice == '2')
            cout <<
            "\n\n\n          Instructions:\n\n          Enter the player names then press enter to proceed with the game.\n          To select the square you would like to place your marker enter the square name followed by the ENTER key.\n          Repeat this until a player has three of their markers in a row. This can be in any direction.\n\n          Enjoy.\n\n\n"
            << endl;

        else if (userChoice == '3')
            music();
        else if (userChoice == '4')
            colour();
        else if (userChoice == '5')
            cout << "\n\n\n          Made in Visual Studios using C++\n          Error code returned as = 0x80070002\n          Successful Code Return Game Played = 1073741510 (0xc000013a).\n          Version 2.0\n\n" << endl;
        else if (userChoice == '6')
            cout << "\n\n\n          Created by Jamie Clifford.\n\n " << endl;
        else if (userChoice == '7')
        {
            cout << "\n\n\n          Thank you for playing, Good bye" << endl;
        }
        else
            cout << "\n\n\n          Error - Please choose again" << endl;

    } while (userChoice != '7');


}

//Background Music Option to Play or Turn Off//
void music()
{
    char userChoice = 'o';
    cout << " \n\n\n         Would you like to turn music ON or OFF?\n\n" << endl
        << "          1. ON" << endl
        << "          2. OFF\n";
    cout << "\n          Please make a choice:  ";
    cin >> userChoice;
    system("CLS");
    if (userChoice == '1')
        PlaySound(TEXT("Tetris.wav"), NULL, SND_ASYNC);
    else if (userChoice == '2')
        PlaySound(NULL, NULL, SND_ASYNC);

}

//Change the Background and Text Colours//
void colour()
{
    char userChoice = 'o';
    cout << "\n\n\n          What Colour Background and Text Would You Like?\n\n" << endl
         << "          1. Black Background and White Text" << endl
         << "          2. Grey Background and Black Text" << endl
         << "          3. Green Background and Light Yellow Text" << endl
         << "          4. Blue Background and Light Aqua Text" << endl
         << "          5. Red Background and Green Text" << endl;
    cout << "\n          Please make a choice:  ";
        cin >> userChoice;
        system("CLS");
    if (userChoice == '1')
        system("Color 07"); //Black Background and White Text//
    else if (userChoice == '2')
        system("Color 70"); //Grey Background and Black Text//
    else if (userChoice == '3')
        system("Color 2E"); //Green Background and Yellow Text//
    else if (userChoice == '4')
        system("Color 1B"); //Blue Background and Light Aqua Text//
    else if (userChoice == '5')
        system("Color 4A"); //Red Background and Green Text//
}

//Board//
void Draw()
{
    system("CLS");
    //Board//
    cout << "\n\n\n          Noughts and Crosses \n\n";
    for (int i = 0; i < 3; i++)
    {
        cout << "                 ";
        for (int j = 0; j < 3; j++)
        {
            cout << matrix[i][j] << " ";
        }
        cout << endl;
    }
}

//Player Input//
void Input()
{

    int a;
    rear++;
    player;

    do
    {
        //Beginning of game, where player input their chosen space//

        {   if (player == 'X')
            cout << "\n        It's " << charName << "'s turn. Enter a number:  ";
        else if (player == 'O')
            cout << "\n        It's " << charName2 << "'s turn. Enter a number:  ";
        }
        cin >> a;



        if (a == 1 && matrix[0][0] == '1')
            matrix[0][0] = player;
        else if (a == 2 && matrix[0][1] == '2')
            matrix[0][1] = player;
        else if (a == 3 && matrix[0][2] == '3')
            matrix[0][2] = player;
        else if (a == 4 && matrix[1][0] == '4')
            matrix[1][0] = player;
        else if (a == 5 && matrix[1][1] == '5')
            matrix[1][1] = player;
        else if (a == 6 && matrix[1][2] == '6')
            matrix[1][2] = player;
        else if (a == 7 && matrix[2][0] == '7')
            matrix[2][0] = player;
        else if (a == 8 && matrix[2][1] == '8')
            matrix[2][1] = player;
        else if (a == 9 && matrix[2][2] == '9')
            matrix[2][2] = player;
        else {
            cout << "\n        Invalid number, please try again.\n\n ";
            system("pause");
            rear--;
            cin.ignore();
            cin.get();
            a = -1;
        }


    } while (a == -1);

}

//Function to Switch Players//
void togglePlayer()
{
    if (player == 'X')
        player = 'O';
    else player = 'X';
}

//Checks for a Winner//
char Win()
{
    //first player//
    if (matrix[0][0] == 'X' && matrix[0][1] == 'X' && matrix[0][2] == 'X')
        return 'X';
    if (matrix[1][0] == 'X' && matrix[1][1] == 'X' && matrix[1][2] == 'X')
        return 'X';
    if (matrix[2][0] == 'X' && matrix[2][1] == 'X' && matrix[2][2] == 'X')
        return 'X';

    if (matrix[0][0] == 'X' && matrix[1][0] == 'X' && matrix[2][0] == 'X')
        return 'X';
    if (matrix[0][1] == 'X' && matrix[1][1] == 'X' && matrix[2][1] == 'X')
        return 'X';
    if (matrix[0][2] == 'X' && matrix[1][2] == 'X' && matrix[2][2] == 'X')
        return 'X';

    if (matrix[0][0] == 'X' && matrix[1][1] == 'X' && matrix[2][2] == 'X')
        return 'X';
    if (matrix[2][0] == 'X' && matrix[1][1] == 'X' && matrix[0][2] == 'X')
        return 'X';

    //second player//
    if (matrix[0][0] == 'O' && matrix[0][1] == 'O' && matrix[0][2] == 'O')
        return 'O';
    if (matrix[1][0] == 'O' && matrix[1][1] == 'O' && matrix[1][2] == 'O')
        return 'O';
    if (matrix[2][0] == 'O' && matrix[2][1] == 'O' && matrix[2][2] == 'O')
        return 'O';

    if (matrix[0][0] == 'O' && matrix[1][0] == 'O' && matrix[2][0] == 'O')
        return 'O';
    if (matrix[0][1] == 'O' && matrix[1][1] == 'O' && matrix[2][1] == 'O')
        return 'O';
    if (matrix[0][2] == 'O' && matrix[1][2] == 'O' && matrix[2][2] == 'O')
        return 'O';

    if (matrix[0][0] == 'O' && matrix[1][1] == 'O' && matrix[2][2] == 'O')
        return 'O';
    if (matrix[2][0] == 'O' && matrix[1][1] == 'O' && matrix[0][2] == 'O')
        return 'O';

    return '/';
}

//Main Function to Play the Game//
void play()
{//Players enter their names//
    cout << "\n\n\n          Player 1 enter your name:  ";
    cin >> charName;
    cout << "\n\n\n          Player 2 enter your name:  ";
    cin >> charName2;
    char choice;
    Draw();
Start:
    while (1)
    {
        Input();
        Draw();

        if (Win() == 'X')
        {
            cout << "\n        " << charName << " Wins The Game  ";
            break;
        }
        else if (Win() == 'O')
        {
            cout << "\n        " << charName2 << " Wins The Game " << endl;
            break;
        }
        else if ( rear == 9)
        {
            cout << "           Draw" << endl;
            break;
        }
        togglePlayer();

    } 
    //Choice to play the game again or return to the main menu//
    cout << "\n\n   ";
    cout << "       Do you want to go play again? ";
    cout << "\n\n          1. Yes\n";
    cout << "          2. No\n\n";

    cout << "          ";
    cin >> choice;
    if (choice == '1')
        cout << "\n          Enjoy\n";
    else if (choice == '2')
        mainMenu();
    while (choice != '1');


    { for (int i = 0; i < 3; i++) {
        for (int j = 0; j < 3; j++) {
            matrix[i][j] = i * 3 + (j + 1) + 48;
        }
    }

    rear = 0;
    player = 'X';
    goto Start;


    }

}

Thank you in advance.

Answer 1

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 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;
...
case '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 enums 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.

Answer 2

Another answer here gives some good advanced advice, I'm going to point out some basics

If I was teaching C++, I would give this program an A for effort, but a C- for execution. It is not totally clear by reading the code that the program actually runs as expected.

Goto

Don't use goto unless it is absolutely necessary for error handling. If this was the early years of BASIC or FORTRAN the goto would be ok, but not in C, C++ or other high level languages.

The current use of goto could be avoided by returning to the calling function after the function play() completes. This would also remove the call to mainMenu() since that is the calling function to be returned to.

Global Variables

Generally variables should be declared where they are needed and should be limited to the smallest scope possible, for instance chanName and chanName2 should be declared in the function play() and passed to any other functions that need them.

Global variables should be avoided when possible, they make programming very difficult, it is hard to find where global variables are modified within a program, and they can cause linking errors in larger programs that have more than once source file. They make maintaining a program a nightmare. Two good articles on why global variables are bad are GlobalVariablesAreBad and this stackoverflow question.

Avoid using namespace std

If you are coding professionally you probably should get out of the habit of using the using namespace std; statement. The code will more clearly define where cout and other identifiers are coming from (std::cin, std::cout). As you start using namespaces in your code it is better to identify where each function comes from because there may be function name collisions from different namespaces. The identifiercout you may override within your own classes, and you may override the operator << in your own classes as well. This stack overflow question discusses this in more detail.

Complexity

The function play() is too complex (does too much). Because it is doing too much it has a goto in it which greatly increases the complexity of the function. Complex functions are hard to read, write, debug and maintain. They can be a major cause of logic errors. A good programming practice is to keep breaking down functions into smaller and smaller functions until each function is very simple to write and debug. The complexity of the function also hides the fact that the is a type related logic error in the function

while (choice != '1');
{
    for (int i = 0; i < 3; i++) {
        for (int j = 0; j < 3; j++) {
            matrix[i][j] = i * 3 + (j + 1) + 48;
        }
    }

    rear = 0;
    player = 'X';

}

The semicolon after the while loop prevents any following code from executing.

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.