I'm currently studying Java by myself. The book I'm using (while very good) lacks feedback (obviously). While trying to write this program, I found myself solving most of the problems I've encountered by trial and error and makeshift solutions, which I fear might have led to some haphazard code. A good example would be the initialization of several variables: bestline and bestrow in the Testing class, and bestMove in the Board class.

The code also feels very cumbersome. I would really appreciate it if you could review my code and comment on where I can improve, best practices I should adopt, etc...

import java.util.Scanner;

public class Testing 
    public static void main(String[] args) {
        int turn = 0; // turn is either 0 or 1 , changed with Math.abs(turn-1).
        Board board = new Board ();
        Scanner myScanner = new Scanner(System.in);

for( ; board.scoreBoard(board) == 1000 ; ){

    if (turn == 0){        //X's turn
        int line = myScanner.nextInt(); // get a line
        int row = myScanner.nextInt(); // get a row
        for ( ; board.Square[line][row] != 0 ;line = myScanner.nextInt() , row = myScanner.nextInt() ){ // run while the square at [line][row] is taken,print the line and pick another.
            System.out.println("Square Taken , please pick another.");
        board.Square[line][row] = 1; //make the move (1 means X)

    if (turn == 1){        //O's turn
        int lastboard = 100; // O's worst case scenario so it has somthing to compare in the first round. // this will keep the best score that player can get
        int bestline=0; // no meaning because I had to initialize
        int bestrow =0; // no meaning because I had to initialize

        for(int line = 0 ; line <3 ; line++){    //
            for(int row = 0 ; row < 3 ; row ++){ //go over the entire board
                if(board.Square[line][row]==0){ //if (Square is empty)
                    board.Square[line][row] = 2; // place 2 (2 means O) , this "tries" a move.
                    int tmpboard = board.scoreBestMove(board, Math.abs(turn-1)); //"scores" the move just made, keeps it in a temporary int.
                    if(tmpboard<=lastboard){ // if the move just made is better than previous best move.
                        lastboard = tmpboard; // keep the new score as the best score.
                        System.out.println(line+" "+row+" this is " +board.scoreBestMove(board, Math.abs(turn-1))); // printing how it scores every move for debugging. // ignore.
                        bestline = line; // keeps the best moves line.
                        bestrow = row;   // keeps the best moves row.
                    board.Square[line][row] = 0; // resets the square.
        board.Square[bestline][bestrow] = 2; // once the loop is over , do the best move you found.
        board.printBoard(board); // print the board.

    turn = Math.abs(turn-1); // change the turn.


import java.math.*;

public class Board {
    int  Square[][] = new int [3][3];

        int  Square[][] = new int [3][3];
// constructor for a new board , makes an int array size 3/3.

public void initializeBoard (Board board){
    for(int i=0 ; i<3 ; i++)
        for(int j=0 ; j<3 ; j++)
            Square[i][j] = 0;

public int scoreBoard (Board board){
        if(     (board.Square[0][0] == 1 & board.Square[0][1] == 1 & board.Square[0][2] == 1) || // 1'st line X
                (board.Square[1][0] == 1 & board.Square[1][1] == 1 & board.Square[1][2] == 1) || // 2'nd line X
                (board.Square[2][0] == 1 & board.Square[2][1] == 1 & board.Square[2][2] == 1) || // 3'rd line X
                (board.Square[0][0] == 1 & board.Square[1][0] == 1 & board.Square[2][0] == 1) || // 1'st row X
                (board.Square[0][1] == 1 & board.Square[1][1] == 1 & board.Square[2][1] == 1) || // 2'nd row X
                (board.Square[0][2] == 1 & board.Square[1][2] == 1 & board.Square[2][2] == 1) || // 3'rd row X
                (board.Square[0][0] == 1 & board.Square[1][1] == 1 & board.Square[2][2] == 1) || // 1'st diag X
                (board.Square[0][2] == 1 & board.Square[1][1] == 1 & board.Square[2][0] == 1) ){ // 2'nd diag X
            return 100; //X win
        if(     (board.Square[0][0] == 2 & board.Square[0][1] == 2 & board.Square[0][2] == 2) || // 1'st line X
                (board.Square[1][0] == 2 & board.Square[1][1] == 2 & board.Square[1][2] == 2) || // 2'nd line X
                (board.Square[2][0] == 2 & board.Square[2][1] == 2 & board.Square[2][2] == 2) || // 3'rd line X
                (board.Square[0][0] == 2 & board.Square[1][0] == 2 & board.Square[2][0] == 2) || // 1'st row X
                (board.Square[0][1] == 2 & board.Square[1][1] == 2 & board.Square[2][1] == 2) || // 2'nd row X
                (board.Square[0][2] == 2 & board.Square[1][2] == 2 & board.Square[2][2] == 2) || // 3'rd row X
                (board.Square[0][0] == 2 & board.Square[1][1] == 2 & board.Square[2][2] == 2) || // 1'st diag X
                (board.Square[0][2] == 2 & board.Square[1][1] == 2 & board.Square[2][0] == 2) ){ // 2'nd diag X
            return 0; // O win
        if (    board.Square[0][0] == 0 || // this "if" gets called if no player won yet, this checks to see if the game is over , it returns 1000 if the game is not over.
                board.Square[0][1] == 0 || 
                board.Square[0][2] == 0 ||
                board.Square[1][0] == 0 ||
                board.Square[1][1] == 0 ||
                board.Square[1][2] == 0 ||
                board.Square[2][0] == 0 ||
                board.Square[2][1] == 0 ||
                board.Square[2][2] == 0){
        return 1000; // game not over
        return 50; // draw , gets to this "if" only if no other if is called.
// get a board: returns 100 if X won , 0 if O won , 50 if draw and 1000 if game not over.

public void printBoard (Board board){
    for(int i=0 ; i<3 ; i++){
        for(int j=0 ; j<3 ; j++){
            System.out.print("|"+ Square[i][j]+ "|");       

public int scoreBestMove (Board board , int turn){
    if(board.scoreBoard(board)!= 1000){ // if(game over)
        return board.scoreBoard(board); // return the score
    int bestmove=50; // no meaning because I had to initialize
    int tmpbestmove; // temoprary int to store the value the recursion returns , so I dont have to call it twice.
switch (turn){
case 0: //X's turn
    bestmove = 0; // makes the bestmove's score X's worst case scenario.
case 1://O's turn
    bestmove = 100;// makes the bestmove's score O's worst case scenario.
for(int i = 0 ; i <3 ; i++){         //
    for (int j = 0 ; j <3 ; j++){    //go over the entire board
        if(board.Square[i][j]==0){   //if (Square is empty)
            case 0: // X's turn
                board.Square[i][j] = 1; // place 1 (1 means X) , this "tries" a move.
                tmpbestmove = board.scoreBestMove(board, Math.abs(turn-1)); // call itself with the board and the other player's turn to rate the move it just made. 
                if(tmpbestmove>bestmove){ //if the move scores better (for X) than any previous moves 
                    bestmove = tmpbestmove; // keep that score in bestmove.
                board.Square[i][j] = 0; // reset that Square to empty.
            case 1: // O's turn
                board.Square[i][j] = 2; // place 2 (2 means O) , this "tries" a move.
                tmpbestmove = board.scoreBestMove(board, Math.abs(turn-1));// call itself with the board and the other player's turn to rate the move it just made.
                if(tmpbestmove<bestmove){ //if the move scores better(for O) than any previous moves 
                    bestmove = tmpbestmove; // keep that score in bestmove.
                board.Square[i][j] = 0; // reset that Square to empty.
    return bestmove; // return the score for the best option , NOTE : this does not return a move , but the score for the callers move.
// gets a board and a turn  , returns the score for the move.
  • \$\begingroup\$ Hi , been getting some great answers though one part remains unreviewed the recursive scoreBestMove , I know beggars can't be choosers but I would really love for that function to get reviewed. \$\endgroup\$
    – Tom
    Commented Sep 26, 2012 at 11:58

3 Answers 3


Welcome to CodeReview. Your concern for the quality of your code is warranted, but don't let that put you down: digging through that book on your own, writing your own code and even exposing it to the Internet's critical eyes are impressive first steps in the right direction.

You seem interested in writing clean code. I can warmly recommend the book Clean Code: A Handbook of Agile Software Craftsmanship (Robert C. Martin). For me, it was a very thought-provoking book that helped me really understand why internal software quality matters (and how to achieve it).

Now, let's get to the Review.

Pulazzo's suggestions are spot on. (As he pointed out, you may want to revisit the concept of object instances and the this keyword).

In addition to the points already made, I'd like to explain some more abstract topics that really drive code quality.

Theoretical Background

The fundamental problem

Programmers like to think of themselves as smart. We tend to solve a problem, marvel at our cleverness, and leave a mess behind without even realizing it. It's only when we revisit our code later (often in the process of trying to add a feature or fix a bug), we realize that reading and understanding code is a lot harder than writing it. There's often too much information to comfortably wrap our heads around.

We then write comments (in your case, almost a comment per line) to help make sense of the whole thing, but in doing so, we add even more information, and this time there isn't even a guarantee for reliability or truthfullness: comments go off faster than milk left out of the fridge at 40°C, rendering the "information" they contain obsolete.

Even worse, comments are often redundant. It's as if we are writing everything twice! A good programmer is a lazy programmer in that she never repeats herself.

(Comments do have their place, for instance in the documentation of public APIs and explaining particularly weird decisions we made.)

The solution

In order to cope with complexity, we need to split it up and simplify it as far as possible. In other words: Divide and conquer. If our classes are small, we can easily spot the responsibilities. If our methods are small, we can easily discern the flow of control. If our lines are short, we can make sense of the statements they contain.

Conciseness isn't the only thing that matters. It's even more important to choose proper names. Without proper names, we are constantly decoding and reconstructing the information that should be apparent from reading the code alone.

Applying it to your Tic Tac Toe

The entry point

Look at your main method. It spans nearly 50 lines and contains six levels of nesting. I tried really hard, but my head nearly exploded trying to comprehend everything you were doing in there.

What if that method were to look like this?

private static final Scanner in = new Scanner(System.in);
private static final PrintStream out = System.out;
private static final Game game = new Game();

public static void main(String args[]) {
    out.println("Welcome to TicTacToe.");
    while (game.isRunning()) {

A lot of effort went into transforming your original code into this readable and expressive form. But where, you ask, has all the code gone? Let's go step by step.

Because I have split everything into separate methods that each do one thing only, I've promoted the local variables to private static final fields. There's your familiar Scanner, plus a reference to System.out so I can use the short form out.println(...) and drop the System (I find it more convenient, but go with what you feel most comfortable with).

The methods that are called from main all look pretty much how you expected them:

private static void informPlayersOfNextTurn() {
    String message = "Player %s, it's your turn. Make your move!\n";
    out.printf(message, game.getCurrentPlayer());

We simply ask the game for the current player and prompt them to make their move.

Note the high level of these method calls... there's not much of an implementation visible here, just a high level view of how the program flows. We'll keep asking for a move until the user enters a valid one:

private static void makeNextMove() {
    Move move = askForMove();
    while (!game.isMoveAllowed(move)) {
        out.println("Illegal move, try again.");
        move = askForMove();

Now, we delve a little deeper. Users aren't usually accustomed to the programming convention of counting from zero, so we'll allow them to enter their moves in familiar terms and subtract 1 to transform them to an appropriate format for use with Java.

private static Move askForMove() {
    int x = askForCoordinate("horizontal");
    int y = askForCoordinate("vertical");
    return new Move(x - 1, y - 1);

Finally, we've arrived at the implementation level: We're using the Scanner to gather some valid input.

private static int askForCoordinate(String coordinate) {
    out.printf("Enter the %s coordinate of the cell [1-3]: ", coordinate);
    while (!in.hasNextInt()) {
        out.print("Invalid number, re-enter: ");
    return in.nextInt();

And that's it! This is all that goes in the Testing class. (I called it Main because that's my personal convention, but Testing is okay too).

What happened to the rest of the code? In Main, we're really only interested in the rough flow of logic and handling the input, so the other responsibilities have been delegated to Game. One of the problems with your original code is that it only has two classes: Testing and Board. But those are too many responsibilities bundled into one.

Tracking game state

public class Game {
    private final Board board = new Board();
    private Player currentPlayer = Player.X;

The game has a Board and tracks the current Player (X has the first move; Player is an enum).

    public boolean isRunning() {
        return !currentPlayer.isWinner && !board.isFull();

Scroll back up and look at our while loop in main. So now it's clear: the game is only running if there isn't a winner yet and the board is not yet full. Otherwise, it's ended.

    public Player getCurrentPlayer() {
        return currentPlayer;

    public boolean isMoveAllowed(Move move) {
        return isRunning() && board.isCellEmpty(move.X, move.Y);

A move is allowed if the game is running and the cell we are trying to put an X or O in is empty.

You can only make a move if the game is still running. If we don't have a winner after making the move, the next player's turn starts:

    public void makeMove(Move move) {
        if (!isRunning()) {
            throw new IllegalStateException("Game has ended!");
        board.setCell(move.X, move.Y, currentPlayer);
        if (!currentPlayer.isWinner) {
            currentPlayer = currentPlayer.getNextPlayer();

When the game has finished, we can check the result:

    public GameResult getResult() {
        if (isRunning()) {
            throw new IllegalStateException("Game is still running!");
        return new GameResult(currentPlayer);

And to print the board to the console, toString is overridden (so out.println(game); will print the game board in its current state):

    public String toString() {
        return board.toString();


Now the only major thing to take a look at is Board.

The tic tac toe board

public class Board {
    private final int LENGTH = 3;
    private final Player[][] cells = new Player[LENGTH][LENGTH];
    private int numberOfMoves = 0;

Instead of your "two-dimensional" array of int, I've used the Player enum to make the code more expressive (and to get rid of all the magic numbers such as 500, 1000 and 50 that are so prevalent in your original code). We keep count of the moves so far to easily tell if the board is full without counting the full cells later.

In the constructor, we fill all the rows with cells:

    public Board() {
        for (Player[] row : cells)
            Arrays.fill(row, Player.Blank);

The following method is called when the player makes a move. It replaces your three enormous if statements where the conditions spanned multiple lines. Even in this refactored version, some complexity remains. Basically, the approach is:

  • We can base our calculations on the current move,
  • so we only need to examine the current column, row, diagonal and anti-diagonal.
  • If any of those is long enough (3), we have a winner.

If this makes no sense, draw the board on a piece of paper and go through it with a pencil.

    public void setCell(int x, int y, Player player) {
        cells[x][y] = player;
        int row = 0, column = 0, diagonal = 0, antiDiagonal = 0;
        for (int i = 0; i < LENGTH; i++) {
            if (cells[x][i] == player) column++;
            if (cells[i][y] == player) row++;
            if (cells[i][i] == player) diagonal++;
            if (cells[i][LENGTH - i - 1] == player) antiDiagonal++;
        player.isWinner = isAnyLongEnough(row, column, diagonal, antiDiagonal);

    private boolean isAnyLongEnough(int... combinationLengths) {
        return Arrays.binarySearch(combinationLengths, LENGTH) >= 0;

The simplest way I found to find if any of the values is as long as LENGTH is a binary search, which only works when lengths are sorted. You could use a loop just as well.

In game, we sometimes have to check if a cell is empty, so we check if it's on the board and, if it is, whether it is also blank:

    public boolean isCellEmpty(int x, int y) {
        boolean isInsideBoard = x < LENGTH && y < LENGTH && x >= 0 && y >= 0;
        return isInsideBoard && cells[x][y] == Player.Blank;

After nine moves, the board is invariably full:

    public boolean isFull() {
        return numberOfMoves == LENGTH * LENGTH;

Finally, we again override toString, giving us the ability to output the game board. This is still a lot more complex than I would like. Ideally, this might be delegated to a separate GameBoardFormatter class.

    public String toString() {
        final String horizontalLine = "-+-+-\n";
        StringBuilder builder = new StringBuilder();
        for (int row = 0; row < cells.length; row++) {
            for (int column = 0; column < cells[row].length; column++) {
                if (column < cells[row].length - 1)
            if (row < cells.length - 1)
        return builder.toString();


And that's it! Well, almost. You've probably got a pretty good idea what the enum Player and the helper class Move have to look like, but I'll show them for the sake of completeness.

Helper data structures

Some Java experts and professionals will tell you that every field should be protected by getters and setters. I disagree (and Robert C. Martin happens to be of the same opinion): Some classes really have no significant state to protect, so they should be classified as data structures without behaviour and may have public fields. Others will disagree, and using getter-setter methods is fine too.

public enum Player {
    X, O, Blank {
        @Override // to give us a blank space on the board
        public String toString() {
            return " ";

    public boolean isWinner = false;

    public Player getNextPlayer() {
        return this == Player.X ? Player.O : Player.X;


Pretty straightforward: X and O will show up on the game board correctly, so all we need to do is override toString of Player.Blank to give us an empty space. Also, there's a convenience method to give us the next player.

Formatting the game result

public class GameResult {
    private final Player player;

    public GameResult(Player lastPlayer) {
        player = lastPlayer;

    public String toString() {
        String winner = player.isWinner ? player.toString() : "Nobody";
        return String.format("%s won. Thank you for playing.", winner);

A data structure for Player moves

public class Move {

    public final int X;
    public final int Y;
    public Move(int x, int y){
        X = x;
        Y = y;

I hope you find this review useful. I've tried to clearly mark the parts that are purely based on my opinion. Having spent a couple of hours on this, I'd appreciate some feedback in the form of votes and comments.

And sorry for not linking to the more advanced concepts I touched upon: you may need to Google a lot of things. I'll try to come back and put in some links some time.

  • \$\begingroup\$ Thank you for the answer! I am really grateful for your time and effort, this is really taking it to another level. you sir ,deserve a medal. again , this is a much better review than I ever thought I would get , coherent and constructive. thank you! \$\endgroup\$
    – Tom
    Commented Sep 26, 2012 at 9:17
  • \$\begingroup\$ @Thank you, Tom. One thing I neglected in my answer is the artificial intelligence in your implementation. I'm glad you found the review useful anyway. \$\endgroup\$
    – Adam
    Commented Sep 27, 2012 at 20:40

Here are some simple fixes you should make to the beginning of the Board class. I'll let someone else tackle another part.

public class Board {
    int  Square[][] = new int [3][3];

        int  Square[][] = new int [3][3];
// constructor for a new board , makes an int array size 3/3.

public void initializeBoard (Board board){
    for(int i=0 ; i<3 ; i++)
        for(int j=0 ; j<3 ; j++)
            Square[i][j] = 0;

In the above code, do the following:

  1. Prefer lowercase variable names.

  2. Eliminate the array construction in the constructor that you don't use. Since you declare it there, it goes out of scope at the end of the constructor. It temporarily hides your instance variable of the same name, but it isn't referenced and isn't necessary.

  3. Don't pass board into initializeBoard() since you don't use it and is confusing.

  4. Consider moving the code of initializeBoard into Board(). Ask yourself: is there ever a time I would want an uninitialized board? If the answer is no, don't require the caller to initialize it.

  5. Consider eliminating the initializeBoard code. As you can see here, primitive types in Java have default values. For int, the value is 0. Therefore setting each element of the square array to 0 does nothing.

The resulting code would look like this:

public class Board {
    int square[][] = new int[3][3];
    Board() {}

Since I changed the name of your variable from "Square" to "square", you'll need to fix the references in the rest of your code.

Also, consider introducing enums for magic constants in your code and use them instead.

public enum SquareValue {

public enum BoardValue {

    public int value;

    BoardValue(int value) {
        this.value = value;   

It will make it more readable. Of course we'll have to revisit the Board code I have above.

public class Board {
    SquareValue[][] square = new SquareValue[3][3];
    Board() {
        for (int i=0; i<3; i++) {
            for (int j=0; j<3; j++) {
                square[i][j] = SquareValue.Blank;

Then, since scoreBoard is an instance method, don't pass a board. Instead use the board of the current instance. I would also change it to getScore() to better reflect the purpose.

public int getScore() { ... }

Also, refactor some of your code into smaller pieces. For example, create the following functions on the Board class.

public boolean isWinningBoard(SquareValue value) { ... }
public boolean isFullBoard() { ... }

You could also break down isWinningBoard using additional functions. If you don't expect these to be called from outside this class, make them private or protected.

private boolean isWinningRow(int row, SquareValue value) { ... }
private boolean isWinningColumn(int column, SquareValue value) { ... }

Using those functions, my getScore() function that replaces your scoreBoard function would look like this:

public BoardValue getScore() {
    if (isWinningBoard(SquareValue.X)) {
        return BoardValue.X_Wins;
    else if (isWinningBoard(SquareValue.O)) {
        return BoardValue.O_Wins;
    else if (isFullBoard()) {
        return BoardValue.Draw;
    else {
        return BoardValue.GameNotOver;
  • \$\begingroup\$ You would also need isWinningDiagonal() \$\endgroup\$ Commented Sep 25, 2012 at 18:34
  • \$\begingroup\$ Of course. This wasn't meant to be complete. I would probably name it hasWinningDiagonal(SquareValue value) since there are 2 diagonals to check. There are many ways to slice and dice that big scoreBoard method. \$\endgroup\$
    – andykellr
    Commented Sep 25, 2012 at 18:49
  • \$\begingroup\$ I'm glad you found it to be helpful. Have fun! \$\endgroup\$
    – andykellr
    Commented Sep 25, 2012 at 19:00
  • \$\begingroup\$ WOW! thank you for the answer! this makes the code so much more readable , also it actually helped me understand some of the concepts I had not fully grasped. I did not expect such a detailed answer =).-edited for grammer \$\endgroup\$
    – Tom
    Commented Sep 25, 2012 at 19:05

Quick tip about your Board.scoreBoard(...) method:

  1. It was hard to read because of all the lines.
  2. There was a bunch of duplicate code.
  3. It looked like you were using & instead of && -- & is the bitshift operator.

Hopefully I didn't misunderstand the code in that method, but here's another way to write it that (IMHO :) is clearer. The code here is slower than your code, but clearness is always more important than speed in Java, unless you're writing real time code (i.e. robotics) -- and actually, even then, it's still more important.

public int scoreBoard(Board board) {
    if( checkLines(board, 1) ) { 
        return 100;
    else if( checkLines(board, 2) ) { 
        return 0;

    for( int a = 0; a < 3; ++a ) { 
        for( int b = 0; b < 3; ++b ) { 
            if( board.Square[a][b] == 0 ) { 
                return 1000;

    return 50;

private boolean checkLines(Board board, int player ) { 
    for( int x = 0, r = 0; r < 3; ++r, x = 0) {
        if( board.Square[r][x] == player && board.Square[r][++x] == player && board.Square[r][++x] == player ) { 
            return true;
        x = 0;
        if( board.Square[x][r] == player && board.Square[++x][r] == player && board.Square[++x][r] == player ) { 
            return true;

    if( board.Square[0][0] == player && board.Square[1][1] == player && board.Square[2][2] == player ) { 
        return true;
    if( board.Square[0][2] == player && board.Square[1][1] == player && board.Square[2][0] == player ) { 
        return true;
    return false;
  • \$\begingroup\$ thank you for the answer , seems like I failed to notice the &--&& issue, now I am confused as to why my program is still working even though its using "&" instead of "&&". \$\endgroup\$
    – Tom
    Commented Sep 26, 2012 at 9:24
  • 1
    \$\begingroup\$ Wow, something new every day: this confirms that & is a bitshift operator. But this stackoverflow answer clarifies that bitshift operators do work with booleans, as specified in this section of the JLS!! \$\endgroup\$
    – Marco
    Commented Sep 26, 2012 at 11:53
  • \$\begingroup\$ BTW, the effect of using & was that the entire line is evaluated, instead of the expression 'short-cutting' when it determined that the first (boolean) expresion was false. \$\endgroup\$
    – Marco
    Commented Sep 26, 2012 at 11:59

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.