HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Tic Tac Toe Android game

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
androidtoetictacgame

Problem

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 M

Solution

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.

Code Snippets

if(player==0)
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();
}
b[x][y].setText(player.getSymbol());
check[x][y] = player;
player = player.next();
checkBoard();
b[x][y].setText(symbol[player]);
check[x][y] = player;
player = (player + 1)%2;
checkBoard();

Context

StackExchange Code Review Q#84137, answer score: 4

Revisions (0)

No revisions yet.