2
\$\begingroup\$

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");
    }
}
\$\endgroup\$
1
\$\begingroup\$

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;
    }
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for your answer. I will take a bit time and try to implement everything. \$\endgroup\$ – Florian Walther Dec 11 '17 at 11:50
  • 1
    \$\begingroup\$ What do you think about me saving the Button texts into a String each time i check for the winner, like i do in my example? Is that too inefficient? \$\endgroup\$ – Florian Walther Dec 12 '17 at 8:15
  • \$\begingroup\$ @FlorianWalther You should learn about the MVC / MVVM pattern. This basically says that you hold the state in some model classes and have your business logic working on them. The view (which is your buttons) only displays the current content (aka state) of the model and triggers the business logic. \$\endgroup\$ – Timothy Truckle Dec 12 '17 at 8:24
  • \$\begingroup\$ Is there also a name for the way it i do it? And why are most tutorials built like my approach? I often hear about these design patterns but then i usualy see the code the way i wrote it. Is it more used for real apps with more complexity? \$\endgroup\$ – Florian Walther Dec 12 '17 at 8:44
  • \$\begingroup\$ @FlorianWalther "Is there also a name for the way it i do it?" You mean apart from "wrong"? ;o) Only mixing layers comes to my mind... \$\endgroup\$ – Timothy Truckle Dec 12 '17 at 10:13

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.