2
\$\begingroup\$

I'm trying to learn some OOP programming using Java, so I've created TicTacToe. Could you give me some advice on how I can improve this code?

Player.java:

package com.kamil.dude.tactoe;

import java.util.Random;
import java.util.Scanner;

public class Player {
public char currentPlayer;

public Player() {
    Random generator = new Random();
    currentPlayer = (generator.nextBoolean()) == true ? 'O' : 'X';
}

public void setPlayer() {
    currentPlayer = (currentPlayer == 'O') ? 'X' : 'O';
}

public int getInput(int size) {
    Scanner in = new Scanner(System.in);
    int fieldNumber = in.nextInt();
    while (fieldNumber < 1 || fieldNumber > size) {
        fieldNumber = in.nextInt();
    }
    return fieldNumber;
}
}

GameBoard.java:

package com.kamil.dude.tactoe;

public class GameBoard {
private Player player;
private char gameBoard[]; // I want to represent gameBoard as an array of buttons
private int fieldNumber;
private int filledFieldsNumber;
private final int lengthOfGameBoardSide;
private final int bottomLeftCornerIndex; // helps to check slash lines
private int size;

public GameBoard(int size) {
    this.lengthOfGameBoardSide = (int) Math.sqrt(size);
    this.bottomLeftCornerIndex = size - ((int) Math.sqrt(size) - 1);
    this.size = size;
    player = new Player();
    gameBoard = new char[size];
    for (int i = 0; i < size; ++i) {
        gameBoard[i] = '0';
    }
}

public void draw() {
    for (int i = 0, charsInRow = 0; i < size; ++i, ++charsInRow) {
        if (charsInRow == lengthOfGameBoardSide) {
            charsInRow = 0;
            System.out.println();
        }
        if (gameBoard[i] == '0') {
            if (i < 9)
                System.out.print(i + 1 + "  ");
            else
                System.out.print(i + 1 + " ");
        }
        else if (gameBoard[i] == 'O')
            System.out.print("O  ");
        else
            System.out.print("X  ");
    }
    System.out.println();
}

public void checkInput() {
    fieldNumber = player.getInput(size);
    if (player.currentPlayer == 'O' && isFieldFree(fieldNumber)) {
        gameBoard[fieldNumber - 1] = 'O';
        ++filledFieldsNumber;
        player.setPlayer();
    }   
    else if (player.currentPlayer == 'X' && isFieldFree(fieldNumber)) {
        gameBoard[fieldNumber - 1] = 'X';
        ++filledFieldsNumber;
        player.setPlayer();
    }   
}

private boolean isFieldFree(int fieldNumber) {
    return (gameBoard[fieldNumber - 1] == '0') ? true : false;
}

public boolean isGameEnded() {
    if (checkIfWin() || checkIfDraw())
        return true;
    return false;
}

private boolean checkIfWin() {
    if (checkVerticalLines() || checkHorizontalLines() || checkSlashLines())
        return true;
    return false;
}

private boolean checkVerticalLines() {
    int repeatNumber = 0;
    for (int i = 0; i < lengthOfGameBoardSide; ++i) {
        for (int j = i; j < size && repeatNumber != lengthOfGameBoardSide - 1; j += lengthOfGameBoardSide) {
            if (gameBoard[j] != '0' && gameBoard[j + lengthOfGameBoardSide] == gameBoard[j]) {
                ++repeatNumber;
            } else {
                repeatNumber = 0;
                break;
            }
        }
        if (checkRepeatedSignsNumber(repeatNumber)) 
            return true;
    }
    return false;
}

private boolean checkHorizontalLines() {
    int repeatNumber = 0;
    for (int i = 0; i < bottomLeftCornerIndex; i += lengthOfGameBoardSide) {
        for (int j = i; j < size && repeatNumber != lengthOfGameBoardSide - 1; ++j) {
            if (gameBoard[j] != '0' && gameBoard[j + 1] == gameBoard[j]) {
                ++repeatNumber;
            } else {
                repeatNumber = 0;
                break;
            }
        }
        if (checkRepeatedSignsNumber(repeatNumber)) 
            return true;
    }
    return false;
}

private boolean checkSlashLines() {
    if (checkBackSlash() || checkSlash())
        return true;
    return false;
}

private boolean checkBackSlash() {
    int repeatNumber = 0;
    for (int i = 0; i < size && repeatNumber != lengthOfGameBoardSide - 1; i += lengthOfGameBoardSide + 1) {
        if (gameBoard[i] != '0' && gameBoard[i + (lengthOfGameBoardSide + 1)] == gameBoard[i]) {
            ++repeatNumber;
        } else {
            repeatNumber = 0;
            break;
        }
        if (checkRepeatedSignsNumber(repeatNumber)) 
            return true;
    }
    return false;
}

private boolean checkSlash() {
    int repeatNumber = 0;
    for (int i = 2; i < bottomLeftCornerIndex && repeatNumber != lengthOfGameBoardSide - 1; i += size - bottomLeftCornerIndex) {
        if (gameBoard[i] != '0' && gameBoard[i + (size - bottomLeftCornerIndex)] == gameBoard[i]) {
            ++repeatNumber;
        } else {
            repeatNumber = 0;
            break;
        }
        if (checkRepeatedSignsNumber(repeatNumber)) 
            return true;
    }
    return false;
}

private boolean checkRepeatedSignsNumber(int repeatNumber) {
    if (repeatNumber == lengthOfGameBoardSide - 1) {
        draw();
        player.setPlayer();
        System.out.println("Player " + player.currentPlayer + " wins");
        return true;
    }
    return false;
}

private boolean checkIfDraw() {
    if (filledFieldsNumber == size) {
        draw();
        System.out.println("Draw!");
        return true;
    }
    return false;
}
}

Main.java:

package com.kamil.dude.tactoe;

public class Main {

public static void main(String[] args) {
    GameBoard board = new GameBoard(9); // You can change game board size by giving number of fields
    for(;;) {
        board.draw();
        board.checkInput(); 
        if(board.isGameEnded() == true)
            break;
    }
}
}
\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

You make many comparisons with boolean values, for example:

currentPlayer = (generator.nextBoolean()) == true ? 'O' : 'X';
// ...
if(board.isGameEnded() == true)

There's just no need. You can use logical expressions directly, like this:

currentPlayer = generator.nextBoolean() ? 'O' : 'X';
// ...
if(board.isGameEnded())

Similarly to the previous point, instead of this:

if (checkBackSlash() || checkSlash())
    return true;
return false;

You could write much simpler as:

return checkBackSlash() || checkSlash();

The first time I saw it I thought it's a duplicate condition:

    if (gameBoard[i] == '0') {
        // ...
    }
    else if (gameBoard[i] == 'O')

It's hard to see that the first one is a zero, the second one is a capital-oh. In general it's not good to use string literals throughout the code, it would be better to put these values in constants with descriptive names, that will cleanly eliminate possible confusion.


In this code:

if (player.currentPlayer == 'O' && isFieldFree(fieldNumber)) {
    // ...
}   
else if (player.currentPlayer == 'X' && isFieldFree(fieldNumber)) {

Both conditions have isFieldFree(fieldNumber), so it would be better to move that condition up one level, to avoid duplicating it:

if (isFieldFree(fieldNumber)) {
    if (player.currentPlayer == 'O') {
        // ...
    } else if (player.currentPlayer == 'X') {
        // ...
    }   
}

I suggest to use braces with all if statements, even if there will be a single line inside. Otherwise embarrassing bugs can happen.

\$\endgroup\$
0
\$\begingroup\$

You might like using ternary but i promise you that not everybody out there will. try to keep it simple.

public Player() {
Random generator = new Random();
currentPlayer = (generator.nextBoolean()) == true ? 'O' : 'X';}

Ouch: the above hurts my eyes. Why must you insist on this? as Janos says, keep it simple.

    public Player() {
current Player = PlayerFactory(); }

Then get the player factory to choose the right player with a proper conditional and not a ternary operator. Those things ought to be banned!

  • if possible, where ever you see conditionals, think whether there is some benefit to extracting it out into a separate class.

Take this for example:

public void draw() {
for (int i = 0, charsInRow = 0; i < size; ++i, ++charsInRow) {
    if (charsInRow == lengthOfGameBoardSide) {
        charsInRow = 0;
        System.out.println();
    }
    if (gameBoard[i] == '0') {
        if (i < 9)
            System.out.print(i + 1 + "  ");
        else
            System.out.print(i + 1 + " ");
    }
    else if (gameBoard[i] == 'O')
        System.out.print("O  ");
    else
        System.out.print("X  ");
}
System.out.println();}

very hard to read and understand. There are so many conditionals in there. why? be suspicious of them. You could probably extract abstract them into different methods. And from there extract those methods into different classes. and from there use polymorphism to get a particular class to supply the behaviour that you want. It makes it easier to understand and easier to change. But you need tests to make sure it's all running kosher. which brings me to my next point.

where are your tests? I don't think that you can write good code without good tests.

more serious commentary will require the actual code, and the tests to refactor. it would make an interesting exercise.

hope this helps.

\$\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.