5
\$\begingroup\$

I wrote this as an exercise. I think that I have some repetitive code in the "winner" function but I don't know exactly how to reduce it. Any other general advice would be welcome too.

#include<iostream>
#include<iomanip>

char table[3][3];
int turn=1;

void print_table()
{
    for (int i=0;i<3;i++)
    {
        for (int j=0;j<3;j++)
            std::cout<<std::setw(3)<<table[i][j];
    std::cout<<std::endl;
    }
}

void create_table()
{
    for (int i=0;i<3;i++)
        for (int j=0;j<3;j++)
            table[i][j]='*';
}

void input(int turn)
{
    int x=0,y=0;
    std::cin>>x>>y;
    if(turn%2==1)
        table[x][y]='X';
    else
        table[x][y]='O';
}

char winner()
{
    for(int i=0;i<3;i++)
        if(table[0][i]==table[1][i]&&table[1][i]==table[2][i]&&table[0][i]=='X')
        {
            return 'X';
        }

    for(int i=0;i<3;i++)
        if(table[i][0]==table[i][1]&&table[i][1]==table[i][2]&&table[i][0]=='X')
        {
            return 'X';
        }

        for(int i=0;i<3;i++)
        if(table[0][i]==table[1][i]&&table[1][i]==table[2][i]&&table[0][i]=='O')
        {
            return 'O';
        }

    for(int i=0;i<3;i++)
        if(table[i][0]==table[i][1]&&table[i][1]==table[i][2]&&table[i][0]=='O')
        {
            return 'O';
        }

      if(table[0][0]==table[1][1]&&table[0][0]==table[2][2]&&table[0][0]=='O')
      {
            return 'O';
      }

      if(table[0][0]==table[1][1]&&table[0][0]==table[2][2]&&table[0][0]=='X')
      {
            return 'X';
      }

      return '\0'; ///default
}

int main(){

create_table();
print_table();

while (turn<=9&&winner()=='\0')
{
input(turn);
print_table();
winner();
turn++;
}

if(winner()!='\0')
    std::cout<<std::endl<<"winner:"<<winner();
else
    std::cout<<"Draw.";
}
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

1. Simplified winner() method

You could merge the checks for 'X' and 'O' by returning one of the chars in a match of three in a row:

char winner() {
  for (int i = 0; i < 3; i++) {
    if (table[0][i] == table[1][i] && table[1][i] == table[2][i]) {
      return table[0][i]; // 'X' if all are 'X' and 'O' if all are 'O'
    }
  }

  for (int i = 0; i < 3; i++) {
    if (table[i][0] == table[i][1] && table[i][1] == table[i][2]) {
      return table[i][0];
    }
  }

  if (table[0][0] == table[1][1] && table[1][1] == table[2][2]) {
    return table[1][1];
  }

  if (table[0][2] == table[1][1] && table[1][1] == table[2][0]) { // *
    return table[1][1];
  }

  return '\0';
}

*Note that your code is missing the forward slash diagonal

2. Consider eliminating your global variables

I know this is just a practice project, and your code is fine as it is. However, if you wanted to scale your code, then you've created some unnecessary problems for yourself in the future.

All of your functions depend on your two global variables, which limits you to one game and gets in the way of other features by polluting the global namespace. You could eliminate the globals by taking an object oriented approach or a more functional programming styled approach.

The Object Oriented way

The OO way would involve creating a TicTacToeGame class and making the global variables member variables members of your class.

class TicTacToeGame() {
private:
  char table[3][3];
  int turn;

public:  
  TicTacToeGame() {
    for (int i=0;i<3;i++)
      for (int j=0;j<3;j++)
        table[i][j]='*';

    turn = 1;
  }

  char winner() {}

  void input() {
    // ...
    turn++;
  }

  void print() {}

  int getTurn() {
    return turn;
  }
}

int main() {

  TicTacToeGame game = TicTacToeGame();
  game.print();

  while (game.getTurn() <= 9 && game.winner() == '\0')
  {
    game.input();
    game.print();
    game.winner();
  }

  if(game.winner() != '\0')
    std::cout<<std::endl<<"winner:"<<winner();
  else
    std::cout<<"Draw.";
}

Creating a class comes with a few other benefits. Particularly that you can encapsulate how a Tic-tac-toe game is played which helps with code scalability.

A more functional approach

For the functional approach you can just pass the globals as parameters instead (be sure to pass by reference):

char[][] create_table() {
  // ...
  return table;
}

void input (int turn, char[][] &table) {
  // ...
}

char winner(char[][] &table) {
  // ...
}

int main() {

  int turn = 1;
  char[][] table = create_table();
  print_table(table);

  while (turn <= 9 && winner(table) == '\0') {
    input(turn, table);
    print_table(table);
    winner(table);
    turn++;
  }

  if(winner(table) != '\0')
    std::cout<<std::endl<<"winner:"<<winner(table);
  else
    std::cout<<"Draw.";
}

This way gives you a lot more control over when and where turn and board get declared. You can more easily create multiple games to be played simultaneously. The biggest benefit is that your functions have their dependencies explicitly defined; there are no hidden parameters.


Nice job overall! My second suggestion is mostly just to help with a few concepts with software development, and it isn't really applicable if you intend to keep your Tic-tac-toe game as simple as it is. Keep up the good work.

\$\endgroup\$
-1
\$\begingroup\$

To replace the winner function

char  winner = "\0";

for(int i=0;i<3;i++){
    if(table[0][i]==table[1][i]&&table[1][i]==table[2][i]){
          winner  = table[0][i];
    }

    if(table[i][0]==table[i][1]&&table[i][1]==table[i][2]){
          winner  = table[i][0];
    }        
}

if(table[0][0]==table[1][1]&&table[0][0]==table[2][2]){
  winner  = table[0][0];
}

if(table[2][0]==table[1][1]&&table[0][0]==table[0][2]){
   winner  = table[2][0];
}

return winner;

Summary of changes.

  1. Merge all the loops
  2. Merge the seperate checks for x and o
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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