Tic Tac Toe on Android

Question

Can someone review my Tic Tac Toe Code? I tried to make it as compact as I could. What could I improve?

I save the state of the buttons by adding freezesText="true" in the XML file. This is for saving the state on orientation change.

public class MainActivity extends AppCompatActivity implements View.OnClickListener {

    private Button[][] buttons = new Button[3][3];

    private boolean player1Turn = true;

    private int player1Points;
    private int player2Points;

    private TextView textViewPlayer1;
    private TextView textViewPlayer2;


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

        textViewPlayer1 = findViewById(R.id.text_view_p1);
        textViewPlayer2 = findViewById(R.id.text_view_p2);

        for (int i = 0; i < 3; i++) {
            for (int j = 0; j < 3; j++) {
                String buttonID = "button_" + i + j;
                int resID = getResources().getIdentifier(buttonID, "id", getPackageName());
                buttons[i][j] = findViewById(resID);
                buttons[i][j].setOnClickListener(this);
            }
        }

        Button buttonReset = findViewById(R.id.button_reset);
        buttonReset.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View v) {
                resetGame();
            }
        });
    }

    @Override
    public void onClick(View v) {

        if (!((Button) v).getText().equals("")) {
            return;
        }

        if (player1Turn) {
            ((Button) v).setText("X");
        } else {
            ((Button) v).setText("O");
        }

        if (checkForWin()) {
            if (player1Turn) {
                player1wins();
            } else {
                player2wins();
            }
            updatePointsText();
            startNewGame();
        }

        player1Turn = !player1Turn;

    }


    private boolean checkForWin() {
        String[][] results = new String[3][3];

        for (int i = 0; i < 3; i++) {
            for (int j = 0; j < 3; j++) {
                results[i][j] = buttons[i][j].getText().toString();
            }
        }

        for (int i = 0; i < 3; i++) {
            Log.d("check", "checkForWin: Starting if 1");
            if (results[i][0].equals(results[i][1])
                    && results[i][0].equals(results[i][2])
                    && !results[i][0].equals("")) {
                return true;
            }
            Log.d("check", "checkForWin: Starting if 2");
            if (results[0][i].equals(results[1][i])
                    && results[0][i].equals(results[2][i])
                    && !results[0][i].equals("")) {
                return true;
            }
        }

        if (results[0][0].equals(results[1][1])
                && results[0][0].equals(results[2][2])
                && !results[0][0].equals("")) {
            return true;
        }

        if (results[0][2].equals(results[1][1])
                && results[0][2].equals(results[2][0])
                && !results[0][2].equals("")) {
            return true;
        }

        boolean emptyFieldLeft = false;

        outer:
        for (int i = 0; i < 3; i++) {
            for (int j = 0; j < 3; j++) {
                //  Log.d("loop", "Looping through: " + i + " " + j);
                if (results[i][j].equals("")) {
                    emptyFieldLeft = true;
                    break outer;
                }
            }
        }

        if (!emptyFieldLeft) {
            Toast.makeText(MainActivity.this, "Draw!", Toast.LENGTH_LONG).show();
            startNewGame();
        }

        return false;
    }

    private void player1wins() {
        player1Points++;
        Toast.makeText(MainActivity.this, "Player 1 wins!", Toast.LENGTH_LONG).show();
    }

    private void player2wins() {
        player2Points++;
        Toast.makeText(MainActivity.this, "Player 2 wins!", Toast.LENGTH_LONG).show();
    }

    private void updatePointsText() {
        textViewPlayer1.setText("Player 1: " + player1Points);
        textViewPlayer2.setText("Player 2: " + player2Points);
    }

    private void startNewGame() {
        for (int i = 0; i < 3; i++) {
            for (int j = 0; j < 3; j++) {
                buttons[i][j].setText("");
            }
        }
    }

    private void resetGame() {
        player1Points = 0;
        player2Points = 0;
        updatePointsText();
        startNewGame();
    }

    @Override
    protected void onSaveInstanceState(Bundle outState) {
        super.onSaveInstanceState(outState);

        outState.putInt("player1Points", player1Points);
        outState.putInt("player2Points", player2Points);
        outState.putBoolean("player1Turn", player1Turn);
    }

    @Override
    protected void onRestoreInstanceState(Bundle savedInstanceState) {
        super.onRestoreInstanceState(savedInstanceState);

        player1Points = savedInstanceState.getInt("player1Points");
        player2Points = savedInstanceState.getInt("player2Points");
        player1Turn = savedInstanceState.getBoolean("player1Turn");
    }
}

Answer 1

Thanks for sharing your code.

Naming

Finding good names is the hardest part in programming, so always take your time to think about the names of your identifiers.

Choose you names from the problem domain, not from the technical solution.

You have names like buttons which are taken from the implementation details. You should rather choose names more connected to your business case e.g.: this variable could be named: gameCells.

Naming Conventions

Please read (and follow) the Java Naming Conventions. E.g: you have methods like player1wins() and player2wins(). But methods should start woith a verb so better names would be like reportWinPlayer1()

Magic numbers

your code has some magic numbers. This are literal values with a special meaning like here:

    for (int i = 0; i < 3; i++) {
        for (int j = 0; j < 3; j++) 

You should either use constants with meaningful names like

private static final int GAME_FIELD_SIZE =3;
// ...
    for (int i = 0; i < GAME_FIELD_SIZE; i++) {
        for (int j = 0; j < GAME_FIELD_SIZE; j++) {

or use existing constraints like so:

    for (int i = 0; i < buttons.length; i++) {
        for (int j = 0; j < buttons[0].length; j++) {

The string literal "" is a magic number too. So you should use a constant here too:

private static final String EMPTY_FIELD = "";
//
if (results[i][j].equals(EMPTY_FIELD)) {

Avoid break or continue especially when used with labels

break and continue are the poor mans goto. Code using them is hard to follow and even harder to refactor (that is: improving the codes design without changing its behavior).

Single Responsibility/Separation of Concerns

In your excample you mix business logic with user interaction like this:

// in `checkForWin()`
    if (!emptyFieldLeft) {
        Toast.makeText(MainActivity.this, "Draw!", Toast.LENGTH_LONG).show();

also this method immediately calls startNewGame() which is somehow surprising in a method with the name checkForWin. Both (and most likely the surrounding if should not be located somewhere else.

Code duplication

Your approach has lots of code which is almost the same except some constant values like this:

    if (results[0][0].equals(results[1][1])
            && results[0][0].equals(results[2][2])
            && !results[0][0].equals("")) {
        return true;
    }

    if (results[0][2].equals(results[1][1])
            && results[0][2].equals(results[2][0])
            && !results[0][2].equals("")) {
        return true;
    }

You could make it being the same by introducing a variable for the constant differing:

    int columnIndex=0;
    if (results[0][columnIndex].equals(results[1][1])
            && results[0][columnIndex].equals(results[2][2])
            && !results[0][columnIndex].equals("")) {
        return true;
    }
    columnIndex=2;
    if () {
        return true;
    }

Now you have identical code which you can extract to a parameterized using your IDE's automated refactoring:

  private boolean isOwnedCompleteColumn(int columnIndex){
       return results[0][columnIndex].equals(results[1][1])
            && results[0][columnIndex].equals(results[2][2-columnIndex])
            && !results[0][columnIndex].equals("");
  }

and the duplicated part changes to

    if (isOwnedCompleteColumn(0)) {
        return true;
    }

    if (isOwnedCompleteColumn(2)) {
        return true;
    }