18
\$\begingroup\$

I just started coding 10 days back. This is my first program using classes in C++. It is a two player 4x4 tic tac toe game. It works perfectly. If there are any improvements I can make, please let me know.

#include <iostream>

using namespace std;



class Board
{
char board[4][4];
public:
Board()
{
    for(int i=0;i<4;i++)
    {
        for(int j=0;j<4;j++)
        {
            board[i][j]='_';
        }
    }
}


void printBoard()
{

    for(int i=0;i<4;i++)
    {
        for(int j=0;j<4;j++)
        {
            cout<<board[i][j]<<'|';
        }
    cout<<endl;
    }
cout<<endl;
}
int setPosition(char choice)
{
while(1)
{
int row,column;
cout<<"Please enter the row and column"<<endl;
cin>>row>>column;
if(board[row][column]=='_')
{
board[row][column]=choice;
break;

}
else
{
cout<<"Position is taken"<<endl;

}
}
}
char getPosition()
{
for(int i=0;i<4;i++)
{
    for(int j=0;j<4;j++)
    {
        return board[i][j];
    }
    }

}
int checkHorizontal(char choice)
{


int count;
for(int i=0;i<4;i++)
{
    count=0;
    for(int j=0;j<4;j++)
    {
        if(board[i][j]==choice)
        {
            count++;
        }
        if(count==4)
        {

            return 1;
        }
    }

}
return 0;
}
int checkVertical(char choice)
{
int count;
for(int i=0;i<4;i++)
{
    count=0;
    for(int j=0;j<4;j++)
    {
        if(board[j][i]==choice)
        {
            count++;
        }
        if(count==4)
        {

            return 1;
        }
    }

}
return 0;
}
int checkprincipalDiagonal(char choice)
{
int count=0;
for(int i=0;i<4;i++)
{
    if (board[i][i]==choice)
    {
        count++;
    }

    }
if(count==4)
{

  return 1;
}


return 0;
}
int checkotherDiagonal(char choice)
{
int count=0;
for(int i=0;i<4;i++)
{

    for(int j=0;j<4;j++)
    {

        if((i+j)%3==0 && i!=j)
        {
         if (board[i][j]==choice)
        {
           count++;
        }
        }


    }
}

if(count==4)
{

    return 1;
}
    else
    {
        return 0;
    }
}
int checkDraw()
{
for(int i=0;i<4;i++)
{
    for(int j=0;j<4;j++)
    {
        if(board[i][j]=='_')
        {
            return 1;
            break;
        }

    }
 }
return 0;
}

};
class Player
{
string Name;
char choice;
public:
void setName(string NameIn)
{
 Name=NameIn;
}
void setChoice(char choiceIn)
{
    choice=choiceIn;
}
char getChoice()
{
    return choice;
    cout<<endl<<endl;
}

string getName()
{
    return Name;
    cout<<endl<<endl;
}
};
int main()
{
char choice1,choice2;
string Name1,Name2;
Board b1;
cout<<"The board is printed below:"<<endl;
b1.printBoard();
cout<<endl;
Player p1,p2;
cout<<"Player 1,Please Enter your name"<<endl<<endl;
cin>>Name1;
cout<<endl<<endl<<"Player 2,Please Enter your name"<<endl<<endl;
cin>>Name2;
p1.setName(Name1);
p2.setName(Name2);
cout<<endl<<endl<<"Player 1,Please Enter a character to use"<<endl<<endl;
cin>>choice1;
cout<<endl<<endl<<"Player 2,Please Enter a character to use"<<endl<<endl;
cin>>choice2;
p1.setChoice(choice1);
p2.setChoice(choice2);
while(1)
{
cout<<endl<<endl<<"Your turn,"<<Name1<<endl<<endl;
b1.setPosition(choice1);
b1.getPosition();
b1.printBoard();
if(b1.checkHorizontal(choice1)==1 || b1.checkVertical(choice1)==1|| 
b1.checkprincipalDiagonal(choice1)==1 || b1.checkotherDiagonal(choice1)==1)
{
    cout<<endl<<endl<<"Congrats"<<"\t"<<Name1<<"\t"<<"You have won"<<endl;
    break;
}
cout<<endl<<endl<<"Your turn,"<<Name2<<endl<<endl;
b1.setPosition(choice2);
b1.getPosition();
b1.printBoard();

if(b1.checkHorizontal(choice2)==1 || b1.checkVertical(choice2)==1|| 
b1.checkprincipalDiagonal(choice2)==1 || b1.checkotherDiagonal(choice2)==1)
{
    cout<<endl<<endl<<"Congrats "<<Name2<<" You Have won!"<<endl;
    break;
}
if(b1.checkDraw()==0)
{
    cout<<endl<<endl<<"The match is a draw!"<<endl;
    break;
}


}



}
\$\endgroup\$
  • 9
    \$\begingroup\$ Could you indent code properly? \$\endgroup\$ – Toto Mar 25 '18 at 8:54
  • 3
    \$\begingroup\$ @Toto Isn't that part of a review? \$\endgroup\$ – πάντα ῥεῖ Mar 25 '18 at 9:09
  • 2
    \$\begingroup\$ @πάνταῥεῖ that depends on if it was formatted before it was copied into stack overflow I'd imagine... \$\endgroup\$ – Shadow Mar 26 '18 at 3:44
  • \$\begingroup\$ @Shadow That's easy to check. Since TABs are preserved but not rendered, you can see that in editing mode. \$\endgroup\$ – πάντα ῥεῖ Mar 26 '18 at 8:18
  • \$\begingroup\$ Not really a code review comment. But shouldn't this be called 'tic tac toe tum'? \$\endgroup\$ – AJFaraday Mar 26 '18 at 8:26
23
\$\begingroup\$

Your code is quite good for someone who started 10 days ago. Here are a couple of improvements you should consider:

  1. Your code is hard to read due to inadequate indentation and spacing. Take a look here for the recommended indentation style for C++.
  2. using namespace std; is fine for simple programs like this, but it's a good idea to break out of the habit of using it. It is considered bad practice.
  3. It is always advisable to keep in mind the possibility of functionality of a program to be expanded in the future, even if that seems unlikely in the present. For instance, your current code is for a 4x4 tic tac toe. If you were to change this in the future to a 3x3 or 5x5 tic tac toe, it would involve a significant amount of work (replacing all 4's in the code to the new dimension). You are currently using a magic number and it would be a good idea to declare a const int with the board dimension.
  4. In setPosition(), you have a while(1) loop which you terminate with break. Using break in this manner is considered bad practice. Try using a do-while loop, where the test takes place after each iteration, in order to solve the problem.
  5. You have several methods of type int that return 0 or 1 depending on whether a condition is true or false. I suggest using the bool data type as the return type of these methods, since that's what it was made for.
  6. Having 4 methods (checkHorizontal(), checkVertical(), checkprincipalDiagonal() and checkotherDiagonal()) is unnecessary since their functionality can be combined into 1 method. Try to think of how you can use 1 method with 1 nested loop to conduct all the checks.
  7. Also note that the names checkprincipalDiagonal() and checkotherDiagonal() do not use proper camelCase.

This should set you off in the right direction, happy coding!

\$\endgroup\$
  • 5
    \$\begingroup\$ Good review! I would like to add that 5. is actually common practice in C. It's possible OP has made the common mistake of applying C practices to C++. \$\endgroup\$ – Wingblade Mar 25 '18 at 10:36
  • 2
    \$\begingroup\$ Thank you so much for mentioning #2.....I swear, the number of people that still do this bothers the hell out of me. Until April last year, Google's protobuf library had it in their common header and my team couldn't understand why I was so upset. \$\endgroup\$ – mascoj Mar 25 '18 at 23:36
  • 2
    \$\begingroup\$ With respect to #5, an ever bigger problem is that the function names don't suggest what the return value means. true and false aren't more meaningful for a function named checkDraw(). On the other hand, if a function were named boardIsFull() I'd know exactly the implications of true and false, (or 1 and 0) \$\endgroup\$ – Ben Voigt Mar 26 '18 at 5:25
11
\$\begingroup\$

For coding since 10 days, this program is really impressive. If I had to learn something new and complicated as C++, I'd probably make many more mistakes.

One thing that is always tricky is input and output. Especially input since you have to deal with funny users who enter 1,4 for the coordinates since they don't know that you expect 1 4.

I tried this by accident, and your program responded with:

Please enter the row and column
1,4
_|_|_|_|
a|_|_|_|
_|_|_|_|
_|_|_|_|



Your turn,Roland

Please enter the row and column
Position is taken
Please enter the row and column
Position is taken
Please enter the row and column
Position is taken
Please enter the row and column
Position is taken
...

So there is obviously something wrong. I solved this using the following code, which takes care of many possible edge cases:

void play(Board &board, Player &player) {
    while (true) {
        std::cout << "Please enter the row and column (1..4): ";

        // If a simple string cannot be read, something is seriously
        // broken. Stop the whole program.
        std::string line;
        if (!std::getline(std::cin, line)) {
            std::exit(std::cin.eof() ? 0 : 1);
        }

        // Let the user type the coordinates either as "1 4"
        // or as "1,4" or "1, 4" or any other variant.
        std::replace(line.begin(), line.end(), ',', ' ');

        int row, column;
        if (!(std::stringstream(line) >> row >> column)) {
            std::cout << "Please enter two numbers\n";
            continue;
        }

        // Make sure that the coordinates are correct.
        // Otherwise the program may crash or do something entirely different.
        // This is called "undefined behavior" and it should frighten you.
        row--;
        column--;
        if (!(0 <= row && row < 4 && 0 <= column && column < 4)) {
            std::cout << "Please enter only numbers in the range 1..4\n";
            continue;
        }

        if (board.at(row, column) != '_') {
            std::cout << "Position is already taken\n";
            continue;
        }

        board.playAt(row, column, player);
        return;
    }
}

As you can see, I also did some other changes to the code. But the most important idea here is to read the input line-by-line and then extract the coordinates from the line. In your current code, when I press Enter for many times before entering any number, the program will not give me any feedback.

A similar case is when entering the player data. I rewrote your code to:

Player input(const std::string &defaultName, char defaultSymbol) {
    std::string line;
    Player player;

    std::cout << defaultName << ", please enter your name: ";
    if (std::getline(std::cin, line) && !line.empty()) {
        player.name = line;
    } else {
        player.name = defaultName;
    }

    std::cout << player.name << ", please enter a character to use: ";
    if (std::getline(std::cin, line) && !line.empty()) {
        player.symbol = line[0];
    } else {
        player.symbol = defaultSymbol;
    }

    return player;
}

Here, I also changed from using the >> operator to using std::getline since that makes the program's behavior much more predictable. The operator cin >> choice in your code doesn't consume the Enter key for example. Therefore, when I changed the program to ask for name1, choice1, name2, choice2, the name2 was always entered automatically and was an empty string. This cannot happen when you consistently read all input as lines.

I also provided useful default values, so that the user can just press Enter four times in a row. The above function is called like this:

int main() {
    Player p1 = input("Player 1", 'x');
    Player p2 = input("Player 2", 'o');
    Board board;

    ...
}

This looks really short and brief, just as the program in main should.

For all the above changes, you obviously have to know that these functions like std::getline or std::replace exist and that you need to include another header with #include <algorithm> at the top of your program. This cannot be expected from a beginner, therefore it's always good to ask, as you did here.

For reference, here is the complete program that I wrote based on your really good code.

#include <algorithm>
#include <cstdlib>
#include <iostream>
#include <sstream>

class Player {
public:
    std::string name;
    char symbol{};
};

class Board {
    static constexpr int boardSize = 4;
    static constexpr int winLength = 4;
    char board[boardSize][boardSize]{};
public:
    Board() {
        for (auto &row : board) {
            for (char &cell : row) {
                cell = '_';
            }
        }
    }

    bool isWin(const Player &player) const {
        auto symbol = player.symbol;
        return checkHorizontal(symbol)
               || checkVertical(symbol)
               || checkPrincipalDiagonal(symbol)
               || checkOtherDiagonal(symbol);
    }

    bool isDraw() const {
        for (auto &row : board) {
            for (char cell : row) {
                if (cell == '_') {
                    return false;
                }
            }
        }
        return true;
    }

    int size() const { return boardSize; }

    char at(int row, int col) const { return board[row][col]; }

    void playAt(int row, int col, const Player &player) {
        board[row][col] = player.symbol;
    }

private:
    bool checkHorizontal(char symbol) const {
        for (auto &row : board) {
            int count = 0;
            for (char cell : row) {
                if (cell == symbol) {
                    count++;
                }
            }
            if (count == winLength) {
                return true;
            }
        }
        return false;
    }

    bool checkVertical(char symbol) const {
        for (int i = 0; i < boardSize; i++) {
            int count = 0;
            for (auto &row : board) {
                if (row[i] == symbol) {
                    count++;
                    if (count == winLength) {
                        return true;
                    }
                } else {
                    count = 0;
                }
            }
        }
        return false;
    }

    bool checkPrincipalDiagonal(char symbol) const {
        int count = 0;
        for (int i = 0; i < boardSize; i++) {
            if (board[i][i] == symbol) {
                count++;
                if (count == winLength) {
                    return true;
                }
            } else {
                count = 0;
            }
        }
        return false;
    }

    bool checkOtherDiagonal(char symbol) const {
        int count = 0;
        for (int i = 0; i < boardSize; i++) {
            if (board[i][boardSize - 1 - i] == symbol) {
                count++;
                if (count == winLength) {
                    return true;
                }
            } else {
                count = 0;
            }
        }
        return false;
    }
};

void print(const Board &board) {
    for (int row = 0; row < board.size(); ++row) {
        for (int col = 0; col < board.size(); ++col) {
            std::cout << '|' << board.at(row, col);
        }
        std::cout << "|\n";
    }
}

void play(Board &board, Player &player) {
    while (true) {
        std::cout << "Please enter the row and column (1.." << board.size() << "): ";

        std::string line;
        if (!std::getline(std::cin, line)) {
            std::exit(std::cin.eof() ? 0 : 1);
        }
        std::replace(line.begin(), line.end(), ',', ' ');

        int row, column;
        if (!(std::stringstream(line) >> row >> column)) {
            std::cout << "Please enter two numbers\n";
            continue;
        }

        row--;
        column--;
        if (!(0 <= row && row < board.size() && 0 <= column && column < board.size())) {
            std::cout << "Please enter only numbers in the range 1.." << board.size() << "\n";
            continue;
        }

        if (board.at(row, column) != '_') {
            std::cout << "Position is already taken\n";
            continue;
        }

        board.playAt(row, column, player);
        return;
    }
}

Player input(const std::string &defaultName, char defaultSymbol) {
    std::string line;
    Player player;

    std::cout << defaultName << ", please enter your name: ";
    player.name = std::getline(std::cin, line) && !line.empty() ? line : defaultName;

    std::cout << player.name << ", please enter a character to use: ";
    player.symbol = std::getline(std::cin, line) && !line.empty() ? line[0] : defaultSymbol;

    return player;
}

int main() {
    Player player1 = input("Player 1", 'x');
    Player player2 = input("Player 2", 'o');
    Board board;
    print(board);

    Player *turn = &player1;
    while (true) {
        std::cout << "Your turn, " << turn->name << ".\n";
        play(board, *turn);
        print(board);

        if (board.isWin(*turn)) {
            std::cout << "Congrats, " << turn->name << ", you have won.\n";
            break;
        }

        if (board.isDraw()) {
            std::cout << "The match is a draw!\n";
            break;
        }

        turn = turn == &player1 ? &player2 : &player1;
    }
}

Feel free to change the two constants boardSize and winLength. A board size of 19 and a win length of 5 make an interesting game. Since these two meanings of 4 were not easily distinguishable in your code, it is good style to give such numbers meaningful names.

\$\endgroup\$
  • \$\begingroup\$ Oops, the above code doesn't check all diagonals, only the longest ones. \$\endgroup\$ – Roland Illig Mar 26 '18 at 18:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.