3
\$\begingroup\$

Can you help me with suggestions regarding my code for Tic Tac Toe Game? This is my code, I've tested it and it works, but I feel like it can still be made shorter/clearer. I am not a good coder and it might look ugly to some.

public class MainActivity extends Activity {

    /**
     * Called when the activity is first created.
     */

    @Override
    public void onCreate(Bundle savedInstanceState)
    {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);
        setBoard();
    }

    int check[][];
    int i,j;
    Button b[][];
    int player=0;
    TextView textView;
    Button newGame;

    // Set up the game board.
    private void setBoard()
    {

        b = new Button[4][4];
        check = new int[4][4];


        textView = (TextView) findViewById(R.id.textview1);

        newGame = (Button) findViewById(R.id.newgame);
        newGame.setOnClickListener (new View.OnClickListener(){

            public void onClick(View v)
            {
                if(newGame.isEnabled())
                {
                    textView.setText("Click button to start!");
                    player=0;
                    setBoard();
                }
            }
        });

        b[1][3] = (Button) findViewById(R.id.one);
        b[1][2] = (Button) findViewById(R.id.two);
        b[1][1] = (Button) findViewById(R.id.three);


        b[2][3] = (Button) findViewById(R.id.four);
        b[2][2] = (Button) findViewById(R.id.five);
        b[2][1] = (Button) findViewById(R.id.six);


        b[3][3] = (Button) findViewById(R.id.seven);
        b[3][2] = (Button) findViewById(R.id.eight);
        b[3][1] = (Button) findViewById(R.id.nine);

        for (i = 1; i <= 3; i++) {
            for (j = 1; j <= 3; j++)
                check[i][j] = 2;
        }

        // add the click listeners for each button

        for (i = 1; i <= 3; i++) {
            for (j = 1; j <= 3; j++) {
                b[i][j].setOnClickListener(new MyClickListener(i, j));
                if (!b[i][j].isEnabled()) {
                    b[i][j].setText("");
                    b[i][j].setEnabled(true);
                }
            }
        }
    }

    class MyClickListener implements View.OnClickListener
    {
        int x;
        int y;


        public MyClickListener(int x, int y)
        {
            this.x = x;
            this.y = y;
        }



        public void onClick(View view)
        {
            if (b[x][y].isEnabled())
            {
                b[x][y].setEnabled(false);
                if (player == 0)
                {
                    b[x][y].setText("X");
                    check[x][y] = 0;
                    player = 1;
                    checkBoard();
                } else
                {
                    b[x][y].setText("O");
                    check[x][y] = 1;
                    player = 0;
                    checkBoard();
                }
            }
        }


  // check the board to see if someone has won


  private boolean checkBoard() {
            boolean gameOver = false;
            if (( check[1][1] == 0 && check[2][2] == 0 &&  check[3][3] == 0)
                    || ( check[1][3] == 0 && check[2][2] == 0 &&  check[3][1] == 0)
                    || ( check[1][2] == 0 &&  check[2][2] == 0 &&  check[3][2] == 0)
                    || ( check[1][3] == 0 &&  check[2][3] == 0 &&  check[3][3] == 0)
                    || ( check[1][1] == 0 &&  check[1][2] == 0 &&  check[1][3] == 0)
                    || ( check[2][1] == 0 &&  check[2][2] == 0 &&  check[2][3] == 0)
                    || ( check[3][1] == 0 &&  check[3][2] == 0 &&  check[3][3] == 0)
                    || ( check[1][1] == 0 &&  check[2][1] == 0 &&  check[3][1] == 0)) {
                textView.setText("Player 1: You win!");
                gameOver = true;
            } else if (( check[1][1] == 1 && check[2][2] == 1 && check[3][3] == 1)
                    || ( check[1][3] == 1 && check[2][2] == 1 && check[3][1] == 1)
                    || ( check[1][2] == 1 && check[2][2] == 1 && check[3][2] == 1)
                    || ( check[1][3] == 1 && check[2][3] == 1 && check[3][3] == 1)
                    || ( check[1][1] == 1 && check[1][2] == 1 && check[1][3] == 1)
                    || ( check[2][1] == 1 && check[2][2] == 1 && check[2][3] == 1)
                    || ( check[3][1] == 1 && check[3][2] == 1 && check[3][3] == 1)
                    || ( check[1][1] == 1 &&  check[2][1] == 1 && check[3][1] == 1)) {
                textView.setText("Player 2: You Win!");
                gameOver = true;
            } else {
                boolean empty = false;
                for (i = 1; i <= 3; i++) {
                    for (j = 1; j <= 3; j++) {
                        if (check[i][j] == 2) {
                            empty = true;
                            break;
                        }
                    }
                }
                if (!empty) {
                    gameOver = true;
                    textView.setText("Game over. It's a draw!");

                }
            }if(gameOver)

                for(i=1;i<=3;i++)
                {
                    for(j=1;j<=3;j++)
                    {
                        b[i][j].setEnabled(false);
                    }
                }

            return gameOver;
        }
    }

}
\$\endgroup\$
4
\$\begingroup\$

This is the main problem with your code:

if(player==0)

This one line is the biggest mistake in your code, and you use it everywhere, for instance:

if (player == 0) {
  b[x][y].setText("X");
  check[x][y] = 0;
  player = 1;
  checkBoard();
} else {
  b[x][y].setText("O");
  check[x][y] = 1;
  player = 0;
  checkBoard();
}

There are a number of places you use that type of horrid duplicate structure. Get rid of it. A (more) acceptable alternative is to do something like this:

b[x][y].setText(player.getSymbol());
check[x][y] = player;
player = player.next();
checkBoard();

At a bare minimum, even something like this would still make your code more readable:

b[x][y].setText(symbol[player]);
check[x][y] = player;
player = (player + 1)%2;
checkBoard();

There is no need to check if(player==0) in your code, since you are doing the exact same thing either way - the only difference is that you are trivially modifying some of the parameters. Handle it by making a separate Player class to keep track of the differences those parameters.

Once you have done that, you should refactor out a number of methods from your code, but there is no point until you take care of the first issue.

\$\endgroup\$
  • \$\begingroup\$ Thanks you very much @AjMansfield , this makes sense to me now that u put it in codes ....im new and it hard for me in implementing .. and i will surely do as u said..Ty again. \$\endgroup\$ – Shailang Kharsati Mar 21 '15 at 4:54

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.