Another TicTacToe Implementation

Question

There are already many other TicTacToe implementations but most of them are missing some features like if duplicate input is entered exception and immediate termination of the program occurs board is not updated and so on I tried my level best to bring the proper one

/*                  SWAMI KARUPPASWAMI THUNNAI
                PROPER IMPLEMENTATION OF TIC TAC TOE IN C++14 (IN OBJECT ORIENTED STYLE)
*/


#include<iostream>
#include<string>
#include<map>
using namespace std;

class Players // Used to store the detais of the players
{
protected:
    string player1, player2; // Names of the players are stored here
    char choice1, choice2;  // either X or O
    map<int, char> Board; // the board itself
};
class TicTacToe:protected Players
{
private:
    int player_choices[9]; //arbitary value assumption see assin definition for more details
    bool winp1 = false;
    bool winp2 = false;
public:
    bool set_players(string name1, string name2,char Choice1,char Choice2); // Used to assign player details
    void board(); // display board
    void assign(); // set values
    bool check(int no,char name); // validating the input
    void update(); // used to update the board
};

bool TicTacToe::set_players(string name1, string name2, char Choice1, char Choice2)
{
    player1 = name1;
    player2 = name2;
    choice1 = Choice1;
    choice2 = Choice2;
    if (isalpha(choice1) && isalpha(choice2))
    {
        if(isupper(choice1)&&isupper(choice2))return true;
        else cout << "Upper case characters please for convineance!" << endl;
    }
    else
    {
        return false;
    }
}
// Used to display the board in the console/terminal
void TicTacToe::board()
{
    cout << "Make yourself familiar with the structure of the board" << endl;
    cout << endl;
    cout << "*-----*-----*-----*" << endl;
    cout << "|  0  |  1  |  2  |" << endl;
    cout << "*-----*-----*-----*" << endl;
    cout << "|  3  |  4  |  5  |" << endl;
    cout << "*-----*-----*-----*" << endl;
    cout << "|  6  |  7  |  8  |" << endl;
    cout << "*-----*-----*-----*" << endl;
}

bool TicTacToe::check(int no,char name) // Member Function will check whether the element is taken or not return type = boolean (bool)
{
    if ((no>=0)&&(no<=8)) {
        map<int, char>::iterator check_if_element_exists;
        check_if_element_exists = Board.find(no);
        if (check_if_element_exists != Board.end())
        {
            cout << "The value is already taken by " << check_if_element_exists->second;
            return false;
        }
        else
        {
            Board[no] = name;
            update();
            return true;
        }
    }
    else
    {
        // you may add some conditional statements for clarity
        cout << "Check the board the number is greater or lesser!" << endl;
    }
}
// This function is used to assign the values
/*
Arbitary value usage:
since I have used maps it would need an iterator to access those X and O characters
which would make code lengthy so I have came with a mathematical approach for solving this TicTacToe
by assuming an arbitary values to the players
Player1 = 20
Player2 = 10
*/
void TicTacToe::assign()
{
    for (int i = 0; i < 9; i++)
    {
        int tempVariable;
        bool surity1 = false; // This will make sure non-duplicated entry is enterd!
        bool surity2 = false;
        while (surity1 != true) {
            cout << "Player 1: Enter your no" << endl;
            cin >> tempVariable;
            bool add = check(tempVariable, choice1);
            if (add == true)
            {
                player_choices[tempVariable] = 20; // arbitary value
                surity1 = true;
            }
        }
        cout << endl << endl << "Player 2\'s Choice" << endl;
        while (surity2 != true) {
            cout << "Player 2: Enter your no" << endl;
            cin >> tempVariable;
            bool add = check(tempVariable, choice2);
            if (add == true) {
                player_choices[tempVariable] = 10; // arbitary value
                surity2 = true;
            }
        }
        //For each entry there may be winning or loosing and it must be announted!
        if ((player_choices[0] == 20) && (player_choices[1] == 20) && (player_choices[2] == 20))
        {
            cout << "Player 1 is the Winner";
            winp1 = true;
            break;
        }
        else if ((player_choices[3] == 20) && (player_choices[4] == 20) && (player_choices[5] == 20))
        {
            cout << "Player 1 is the Winner";
            winp1 = true;
            break;
        }
        else if ((player_choices[6] == 20) && (player_choices[7] == 20) && (player_choices[8] == 20))
        {
            cout << "Player 1 is the Winner";
            winp1 = true;
            break;
        }
        //Linear lines - Vertical
        else if ((player_choices[0] == 20) && (player_choices[3] == 20) && (player_choices[6] == 20))
        {
            cout << "Player 1 is the Winner";
            winp1 = true;
            break;
        }
        else if ((player_choices[1] == 20) && (player_choices[4] == 20) && (player_choices[7] == 20))
        {
            cout << "Player 1 is the Winner";
            winp1 = true;
            break;
        }
        else if ((player_choices[2] == 20) && (player_choices[7] == 20) && (player_choices[8] == 20))
        {
            cout << "Player 1 is the Winner";
            winp1 = true;
            break;
        }
        // Diagonal
        else if ((player_choices[0] == 20) && (player_choices[4] == 20) && (player_choices[8] == 20))
        {
            cout << "Player 1 is the Winner";
            winp1 = true;
            break;
        }
        else if ((player_choices[2] == 20) && (player_choices[4] == 20) && (player_choices[6] == 20))
        {
            cout << "Player 1 is the Winner";
            winp1 = true;
            break;
        }
        else
        {
            cout << endl;
        }
        /*
        ============================For Player 2======================================
        */
        if ((player_choices[0] == 10) && (player_choices[1] == 10) && (player_choices[2] == 10))
        {
            cout << "Player 2 is the Winner";
            winp2 = true;
            break;
        }
        else if ((player_choices[3] == 10) && (player_choices[4] == 10) && (player_choices[5] == 10))
        {
            cout << "Player 2 is the Winner";
            winp2 = true;
            break;
        }
        else if ((player_choices[6] == 10) && (player_choices[7] == 10) && (player_choices[8] == 10))
        {
            cout << "Player 2 is the Winner";
            winp2 = true;
            break;
        }
        //Linear lines - Vertical
        else if ((player_choices[0] == 10) && (player_choices[3] == 10) && (player_choices[6] == 10))
        {
            cout << "Player 2 is the Winner";
            winp2 = true;
            break;
        }
        else if ((player_choices[1] == 10) && (player_choices[4] == 10) && (player_choices[7] == 10))
        {
            cout << "Player 2 is the Winner";
            winp2 = true;
            break;
        }
        else if ((player_choices[2] == 10) && (player_choices[5] == 10) && (player_choices[8] == 10))
        {

            cout << "Player 2 is the Winner";
            winp2 = true;
            break;
        }
        // Diagonal
        else if ((player_choices[0] == 10) && (player_choices[4] == 10) && (player_choices[8] == 10))
        {

            cout << "Player 2 is the Winner";
            winp2 = true;
            break;
        }
        else if ((player_choices[2] == 10) && (player_choices[4] == 10) && (player_choices[6] == 10))
        {

            cout << "Player 2 is the Winner";
            winp2 = true;
            break;
        }
        else
        {
            cout << endl;
        }
        if (i == 8) {
            if ((winp1 == false) && (winp2 == false))
            {
                cout << "Draw match!" << endl;
            }
        }
    }
}

void TicTacToe::update()
{
    // Have an eye with those manipulators any change will shakle the structure
    cout << endl;
    cout << "*-----*-----*-----*"<<endl;
    // Not interested in thinking(merely wasting time) in thinking for creating
    // a new for loop where three loop can be shotened like this for{for{}} may be if
    // you are free then you can try it :)
    for (int i = 0; i <=2; i++)// For displaying first row
    {
        map<int, char>::iterator itr;
        itr = Board.find(i);
        if (itr != Board.end())cout << "  " << itr->second << "  ";
        else cout << "     ";
    }
    cout << "\n*-----*-----*-----*" << endl;
    for (int i = 3; i <=5; i++)// For displaying first row
    {
        map<int, char>::iterator itr;
        itr = Board.find(i);
        if (itr != Board.end())cout << "  " << itr->second << "  ";
        else cout << "     ";
    }
    cout << "\n*-----*-----*-----*" << endl;
    for (int i = 6; i <=8; i++)// For displaying first row
    {
        map<int, char>::iterator itr;
        itr = Board.find(i);
        if (itr != Board.end())cout << "  " << itr->second << "  ";
        else cout << "     ";
    }
}

int main()
{
    string p1, p2;
    char c1, c2;
    TicTacToe game;
    cout << "Enter the name of the player1 :" << endl;
    getline(cin, p1);
    cout << "Enter the name of the player2 :" << endl;
    getline(cin, p2);
    cout << "Enter the choice of the player1 :" << endl;
    cin >> c1;
    cout << "Enter the choice of the player2 :" << endl;
    cin >> c2;
    bool assign_player_details;
    assign_player_details = game.set_players(p1, p2, c1, c2);
    if (assign_player_details == true)cout << endl << "All details have been assigned successfully!" << endl;
    else
    {
        cout << "\nSometing went wrong" << endl;
        return -1;
    }
    game.board();
    cout << endl << "Alright let\'s begin" << endl;
    game.assign();
    int stay; cin >> stay;
    return 0;
}

Any suggestions and revision for this code or even comments is welcome!
also posted in github https://github.com/VISWESWARAN1998/TicTacToe-

Answer 1

I see a number of things which may help you improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Pass by const reference where practical

The first two arguments to TicTacToe::set_players() are a std::string but that causes the strings to be duplicated once for the argument and again for the copy. Better would be to make it const std::string & so that only a single copy is made for the class..

Ensure every control path returns a proper value

The TicTacToe::set_players() routine returns false under some set of conditions but then doesn't return anything at all otherwise. This is an error. The code could instead be written like this:

bool TicTacToe::set_players(const std::string &name1, const std::string &name2,
                            char Choice1, char Choice2)
{
    player1 = name1;
    player2 = name2;
    choice1 = Choice1;
    choice2 = Choice2;
    if (isalpha(choice1) && isalpha(choice2)) {
        if(isupper(choice1)&&isupper(choice2)) {
            return true;
        } else {
            std::cout << "Upper case characters please for convineance!\n";
        }
    }
    return false;
}

There is a similar problem with TicTacToe::check().

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Use string concatenation

The program includes these lines:

void TicTacToe::board()
{
    cout << "Make yourself familiar with the structure of the board" << endl;
    cout << endl;
    cout << "*-----*-----*-----*" << endl;
    cout << "|  0  |  1  |  2  |" << endl;
    cout << "*-----*-----*-----*" << endl;
    cout << "|  3  |  4  |  5  |" << endl;
    cout << "*-----*-----*-----*" << endl;
    cout << "|  6  |  7  |  8  |" << endl;
    cout << "*-----*-----*-----*" << endl;
}

Each of those is a separate call to operator<< but they don't need to be. Another way to write that would be like this:

void TicTacToe::board()
{
    std::cout << "Make yourself familiar with the structure of the board\n\n" 
        "*-----*-----*-----*\n" 
        "|  0  |  1  |  2  |\n" 
        "*-----*-----*-----*\n"
        "|  3  |  4  |  5  |\n"
        "*-----*-----*-----*\n"
        "|  6  |  7  |  8  |\n"
        "*-----*-----*-----*\n";
}

This reduces the entire menu to a single call to operator<< because consecutive strings in C++ (and in C, for that matter) are automatically concatenated into a single string by the compiler.

Use better naming

The functions assign and board and check are not well named. What is check checking? A better name would be less ambiguous. Perhaps isOpen() which tells what's being checked and also gives a hint as to what a value of true would signify.

Separate concerns

As mentioned above, the check function is poorly named but part of the problem is that it does two things. It checks to see if a square is open, and if it is, it also places the player's mark at that square. The logic of the program would be much more clear and easier to write if those were two separate functions.

Separate I/O from program logic

Right now every individual function has both game logic and I/O. It's often better design to separate the two so that the game logic is independent of the I/O with the user. It will make the code cleaner and you can more easily re-use the logic if, for example, you wanted to recode this as a GUI version later on.

Use whitespace to improve readability

Lines like this:

if(isupper(choice1)&&isupper(choice2)) {

become much easier to read with a little bit of whitespace:

if (isupper(choice1) && isupper(choice2)) {

Think of the user

At the start of the game, the program asks for the names of the players, but then they are never used! Either use the names or don't ask for them and don't store them.

Use appropriate data structures

Quite a lot of the code seems to be working with the difficult choice of having a std::map as the underlying data structure for the game. This should be a clue that it is probably not the right choice of data structure. A simple std::array or std::vector would both be better choices.

Use aggregation rather than inheritance for composition

The inheritance mechanism is generally suited for "is-a" relationships. For example, if one had a Birthday class, it might reasonably be derived from a Date base class, since a Birthday is a Date. However, a TicTacToe is not really a Player, but is rather constructed from one, so a better design might have two Player objects as private instance variables.

Write a proper constructor

Much of the game initialization logic would make much more sense as a constructor. For instance, the player tokens (X and O in the traditional game) could simply be passed into a constructor as arguments and the game board set to a coherent initial state (presumably all empty squares).

Use const where practical

Several of the member functions don't (or shouldn't) modify the underlying objects. All such functions should be declared const. For example, one could write a display() member function that would have a signature like this:

void TicTacToe::display() const;

Or even better, I think, would be to write a stream inserter:

friend std::ostream& operator<<(std::ostream &out, const TicTacToe &ttt);

Don't repeat yourself

The logic of the assign() function is really much longer and more convoluted than it should be. Ultimately, all you need is a function that checks to see if there are three of any player tokens in a row. This is much easier if you use an array to represent the game board and would be easier to use and understand if you had it just do one thing and name it to clearly identify what that one thing is.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments: "I didn't know that." or "That's bad advice!" My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard. For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

[...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!) In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.

Answer 2

It has been a while since I did any c++, so I'm going to focus more on the concepts rather than the language features themselves. There probably are more concise ways to implement this in c++ than I can illustrate.

Note #1: Typos. You can use a spell checker on code, there is no reason you couldn't. Why I think it would be a good thing to do is because it will highlight

1. Spelling mistakes in comments and variable names
2. Abbreviations, if you notice there is a lot of words highlighted, maybe there is a few too many abbreviations, and would benefit readability if they were given more descriptive names
3. Give you another chance to read through the code in a different light. Sometimes reading code in a different editor or even without one can change your opinion on what is ugly and what looks nice. You might even spot a bug or a chance to improve things.

---

class Players // Used to store the detais of the players

This comment doesn't really achieve much. If I see a class called Players in a file called TicTacToe.cpp, I would assume it contains code relevant to players in the game. In general comments are useful if they explain why a block of code is there, or what it tries to do.

---

string player1, player2; // Names of the players are stored here

Why not call it playerX_name? Then the comment can be removed Self documenting code is always preferred over comments if possible.

---

map<int, char> Board; // the board itself

Why is this in the players class? Shouldn't this be in a class labeled Game or in its own class?

---

int player_choices[9]; //arbitary value assumption see assin definition for more details

This doesn't seem all that arbitrary. If you change it to 1, or 3, will the code still work? I suspect this is not actually needed.

---

bool winp1 = false;

Why shorten the variable name? I can't think of a single reason to shorten this variable from something like player1_wins. Which one would you prefer to see in 6 months time or a year from now, long after you have forgotten the details of the program. Other names that could benefit from more descriptive names include anything with a comment directly beside it describing it.

For instance void board(); // display board If you changed the function name to display_board or displayBoard (whatever convention the code follows) it would make everything that much simpler to read.

---

bool TicTacToe::set_players(string name1, string name2, char Choice1, char Choice2)

This function is weird. It highlights that the game board is strongly coupled with the players. The reason I say this is because this function assigns the names and characters each player uses at the same time for both players. What makes player1 and player2 special and need to be handled all at once? The code in this function could be halved and called twice, once for each player (or for an arbitrary amount of players if the game was a team game with a much larger board for example)

---

bool TicTacToe::set_players(string name1, string name2, char Choice1, char Choice2)
{
    player1 = name1;
    player2 = name2;
    choice1 = Choice1;
    choice2 = Choice2;
    if (isalpha(choice1) && isalpha(choice2))
    {
        if(isupper(choice1)&&isupper(choice2))return true;
        else cout << "Upper case characters please for convineance!" << endl;
    }
    else
    {
        return false;
    }
}

Continuing from my previous point (and ignoring that if it is a lowe case alphabetic character this doesn't returning anything) this could be a function for just one player. Another small piece of note is that even if the character is not an acceptable character choice, it is still assigned to the player. That seems like an unintentional feature/bug.

My suggestion for a first iteration of refactoring is as a constructor for the Player class

bool Player::Player(string player_name, char player_symbol)
{
    name = player_name;
    if (isalpha(player_symbol) && isupper(player_symbol)
    {
        symbol = player_symbol;
        return true;
    }
    cout << "Please use an uppercase character from the alphabet as your symbol." << endl;
    return false;
}

---

Board[no] = name;
update();
return true;

I don't like updating the board here, this seems very much like mixing business logic with input/output, which is generally a bad idea.

---

while (surity1 != true) {
    cout << "Player 1: Enter your no" << endl;
    cin >> tempVariable;
    bool add = check(tempVariable, choice1);
    if (add == true)
    {
        player_choices[tempVariable] = 20; // arbitary value
        surity1 = true;
    }
}
cout << endl << endl << "Player 2\'s Choice" << endl;
while (surity2 != true) {
    cout << "Player 2: Enter your no" << endl;
    cin >> tempVariable;
    bool add = check(tempVariable, choice2);
    if (add == true) {
        player_choices[tempVariable] = 10; // arbitary value
        surity2 = true;
    }
}

What happens if on player1's go, they win? The game should be over, but player2 still gets to try and make a futile move.

---

<Heap of repeated code>

This is the main chunk of code that needs to be moved to a function. By moving to a function called has_player_won(Player.symbol) you can just return true if any of the conditions have been met, half the repeated code for player2, and remove the need for the values 10 and 20. I reckon you would go from about 120 lines of code to 40ish. After removing the duplicated stuff, there might be other repeated sections that become clear and more obvious to reduce down.

---

There is a decent chunk of text here, so TL;DR is rename variables so that they are more descriptive, don't treat players any different from one another and make functions that represent that ideal, and finally removing duplicated code often gives leeway to notice patterns and simplify code even more.

I hope you take on some of the points and post a follow up, I'm very interested in seeing more from you. Good work!