Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am creating a TicTacToe game for my college project, and when I finished the code for computer AI, I ended up with a big chunk of code.

It allows the computer to make the winning move, stopping the player from winning and making a random move if the computer can't make any winning move or stop the player from winning. The code I have contains a series of if and else if statements.

I was wondering if it would be possible to reduce this amount of code in any way. I have tried putting one of the if statements in a method and then calling the method several times, but this does not work because the else statement does not run.

public void AI(){
        count++;
        if(buttons[1].getText().equals("O") && buttons[2].getText().equals("O") && buttons[3].getText().equals("")){
            buttons[3].setText("O");
            buttons[3].setEnabled(false);
        } else if(buttons[4].getText().equals("O") && buttons[5].getText().equals("O") && buttons[6].getText().equals("")){
            buttons[6].setText("O");
            buttons[6].setEnabled(false);
        } else if(buttons[7].getText().equals("O") && buttons[8].getText().equals("O") && buttons[9].getText().equals("")){
            buttons[9].setText("O");
            buttons[9].setEnabled(false);                
        } 

        else if(buttons[2].getText().equals("O") && buttons[3].getText().equals("O") && buttons[1].getText().equals("")){
            buttons[1].setText("O");
            buttons[1].setEnabled(false);                
        } else if(buttons[5].getText().equals("O") && buttons[6].getText().equals("O") && buttons[4].getText().equals("")){
            buttons[4].setText("O");
            buttons[4].setEnabled(false);                
        } else if(buttons[8].getText().equals("O") && buttons[9].getText().equals("O") && buttons[7].getText().equals("")){
            buttons[7].setText("O");
            buttons[7].setEnabled(false);                
        }

        else if(buttons[1].getText().equals("O") && buttons[3].getText().equals("O") && buttons[2].getText().equals("")){
            buttons[2].setText("O");
            buttons[2].setEnabled(false);                
        } else if(buttons[4].getText().equals("O") && buttons[6].getText().equals("O") && buttons[5].getText().equals("")){
            buttons[5].setText("O");
            buttons[5].setEnabled(false);                
        } else if(buttons[7].getText().equals("O") && buttons[9].getText().equals("O") && buttons[8].getText().equals("")){
            buttons[8].setText("O");
            buttons[8].setEnabled(false);                
        }

        else if(buttons[1].getText().equals("O") && buttons[4].getText().equals("O") && buttons[7].getText().equals("")){
            buttons[7].setText("O");
            buttons[7].setEnabled(false);                
        } else if(buttons[2].getText().equals("O") && buttons[5].getText().equals("O") && buttons[8].getText().equals("")){
            buttons[4].setText("O");
            buttons[4].setEnabled(false);                
        } else if(buttons[3].getText().equals("O") && buttons[6].getText().equals("O") && buttons[9].getText().equals("")){
            buttons[9].setText("O");
            buttons[9].setEnabled(false);                
        }

        else if(buttons[4].getText().equals("O") && buttons[7].getText().equals("O") && buttons[1].getText().equals("")){
            buttons[1].setText("O");
            buttons[1].setEnabled(false);                
        } else if(buttons[5].getText().equals("O") && buttons[8].getText().equals("O") && buttons[2].getText().equals("")){
            buttons[2].setText("O");
            buttons[2].setEnabled(false);                
        } else if(buttons[6].getText().equals("O") && buttons[9].getText().equals("O") && buttons[3].getText().equals("")){
            buttons[3].setText("O");
            buttons[3].setEnabled(false);                
        }

        else if(buttons[1].getText().equals("O") && buttons[7].getText().equals("O") && buttons[4].getText().equals("")){
            buttons[4].setText("O");
            buttons[4].setEnabled(false);                
        } else if(buttons[2].getText().equals("O") && buttons[8].getText().equals("O") && buttons[5].getText().equals("")){
            buttons[5].setText("O");
            buttons[5].setEnabled(false);                
        } else if(buttons[3].getText().equals("O") && buttons[9].getText().equals("O") && buttons[6].getText().equals("")){
            buttons[6].setText("O");
            buttons[6].setEnabled(false);                
        }

        else if(buttons[1].getText().equals("O") && buttons[5].getText().equals("O") && buttons[9].getText().equals("")){
            buttons[9].setText("O");
            buttons[9].setEnabled(false);                
        } else if(buttons[5].getText().equals("O") && buttons[9].getText().equals("O") && buttons[1].getText().equals("")){
            buttons[1].setText("O");
            buttons[1].setEnabled(false);                
        } else if(buttons[1].getText().equals("O") && buttons[9].getText().equals("O") && buttons[5].getText().equals("")){
            buttons[5].setText("O");
            buttons[5].setEnabled(false);                
        }

        else if(buttons[3].getText().equals("O") && buttons[5].getText().equals("O") && buttons[7].getText().equals("")){
            buttons[7].setText("O");
            buttons[7].setEnabled(false);                
        } else if(buttons[7].getText().equals("O") && buttons[5].getText().equals("O") && buttons[3].getText().equals("")){
            buttons[3].setText("O");
            buttons[3].setEnabled(false);                
        } else if(buttons[7].getText().equals("O") && buttons[3].getText().equals("O") && buttons[5].getText().equals("")){
            buttons[5].setText("O");
            buttons[5].setEnabled(false);                
        }

        else if(buttons[1].getText().equals("X") && buttons[2].getText().equals("X") && buttons[3].getText().equals("")){
            buttons[3].setText("O");
            buttons[3].setEnabled(false);
        } else if(buttons[4].getText().equals("X") && buttons[5].getText().equals("X") && buttons[6].getText().equals("")){
            buttons[6].setText("O");
            buttons[6].setEnabled(false);                
        } else if(buttons[7].getText().equals("X") && buttons[8].getText().equals("X") && buttons[9].getText().equals("")){
            buttons[9].setText("O");
            buttons[9].setEnabled(false);                
        } 

        else if(buttons[2].getText().equals("X") && buttons[3].getText().equals("X") && buttons[1].getText().equals("")){
            buttons[1].setText("O");
            buttons[1].setEnabled(false);                
        } else if(buttons[5].getText().equals("X") && buttons[6].getText().equals("X") && buttons[4].getText().equals("")){
            buttons[4].setText("O");
            buttons[4].setEnabled(false);                
        } else if(buttons[8].getText().equals("X") && buttons[9].getText().equals("X") && buttons[7].getText().equals("")){
            buttons[7].setText("O");
            buttons[7].setEnabled(false);                
        }

        else if(buttons[1].getText().equals("X") && buttons[3].getText().equals("X") && buttons[2].getText().equals("")){
            buttons[2].setText("O");
            buttons[2].setEnabled(false);                
        } else if(buttons[4].getText().equals("X") && buttons[6].getText().equals("X") && buttons[5].getText().equals("")){
            buttons[5].setText("O");
            buttons[5].setEnabled(false);                
        } else if(buttons[7].getText().equals("X") && buttons[9].getText().equals("X") && buttons[8].getText().equals("")){
            buttons[8].setText("O");
            buttons[8].setEnabled(false);                
        }

        else if(buttons[1].getText().equals("X") && buttons[4].getText().equals("X") && buttons[7].getText().equals("")){
            buttons[7].setText("O");
            buttons[7].setEnabled(false);                
        } else if(buttons[2].getText().equals("X") && buttons[5].getText().equals("X") && buttons[8].getText().equals("")){
            buttons[8].setText("O");
            buttons[8].setEnabled(false);                
        } else if(buttons[3].getText().equals("X") && buttons[6].getText().equals("X") && buttons[9].getText().equals("")){
            buttons[9].setText("O");
            buttons[9].setEnabled(false);                
        }

        else if(buttons[4].getText().equals("X") && buttons[7].getText().equals("X") && buttons[1].getText().equals("")){
            buttons[1].setText("O");
            buttons[1].setEnabled(false);                
        } else if(buttons[5].getText().equals("X") && buttons[8].getText().equals("X") && buttons[2].getText().equals("")){
            buttons[2].setText("O");
            buttons[2].setEnabled(false);                
        } else if(buttons[6].getText().equals("X") && buttons[9].getText().equals("X") && buttons[3].getText().equals("")){
            buttons[3].setText("O");
            buttons[3].setEnabled(false);                
        }

        else if(buttons[1].getText().equals("X") && buttons[7].getText().equals("X") && buttons[4].getText().equals("")){
            buttons[4].setText("O");
            buttons[4].setEnabled(false);                
        } else if(buttons[2].getText().equals("X") && buttons[8].getText().equals("X") && buttons[5].getText().equals("")){
            buttons[5].setText("O");
            buttons[5].setEnabled(false);                
        } else if(buttons[3].getText().equals("X") && buttons[9].getText().equals("X") && buttons[6].getText().equals("")){
            buttons[6].setText("O");
            buttons[6].setEnabled(false);                
        }

        else if(buttons[1].getText().equals("X") && buttons[5].getText().equals("X") && buttons[9].getText().equals("")){
            buttons[9].setText("O");
            buttons[9].setEnabled(false);                
        } else if(buttons[5].getText().equals("X") && buttons[9].getText().equals("X") && buttons[1].getText().equals("")){
            buttons[1].setText("O");
            buttons[1].setEnabled(false);                
        } else if(buttons[1].getText().equals("X") && buttons[9].getText().equals("X") && buttons[5].getText().equals("")){
            buttons[5].setText("O");
            buttons[5].setEnabled(false);                
        }

        else if(buttons[3].getText().equals("X") && buttons[5].getText().equals("X") && buttons[7].getText().equals("")){
            buttons[7].setText("O");
            buttons[7].setEnabled(false);                
        } else if(buttons[7].getText().equals("X") && buttons[5].getText().equals("X") && buttons[3].getText().equals("")){
            buttons[3].setText("O");
            buttons[3].setEnabled(false);                
        } else if(buttons[7].getText().equals("X") && buttons[3].getText().equals("X") && buttons[5].getText().equals("")){
            buttons[5].setText("O");
            buttons[5].setEnabled(false);                
        }

        else if(buttons[1].getText().equals("X") && buttons[5].getText().equals("O") && buttons[9].getText().equals("X")) {
            buttons[6].setText("O");
            buttons[6].setEnabled(false);            
        }    

        else if(buttons[3].getText().equals("X") && buttons[5].getText().equals("O") && buttons[7].getText().equals("X")) {
            buttons[4].setText("O");
            buttons[4].setEnabled(false);            
        }

        else if(buttons[5].getText().equals("")){
            buttons[5].setText("O");
            buttons[5].setEnabled(false);                
        }

        else if(buttons[1].getText().equals("")){
            buttons[1].setText("O");
            buttons[1].setEnabled(false);                
        }
        else {
            if(count >= 9)
                checkWin();
            else
                RandomMove();
        }

        checkWin();

    }

Edited: The if statements I have check for all the different possibilities that computer can make a winning move or stop the player from winning.

edited 3:

for(int i=0; i<=3; i++){
    if (buttons[i*3].getText().equals("X") && buttons[i*3+1].getText().equals("X") && buttons[i*3+2].getText().equals(""))
        buttons[i*3+2].setText("O");
        win = true;
    }
share|improve this question
    
Sometimes it's possible to use a dictionary in place of a series of if statements. Have you considered this approach? –  gjdanis Mar 17 at 20:17
4  
At a glance, your AI logic seems very tightly coupled with your UI /user interface. –  Mat's Mug Mar 17 at 20:17
1  
@Seeker: a dictionary, or hash table, is a data structure that maps a distinct key to a value. Dictionaries often can help simplify the representation of your problem, and are a great tools for many applications. In Java, I think you need to implement Map (the equivalent to the data structure I'm describing). –  gjdanis Mar 17 at 20:20
1  
@gjdanis I am not sure how or in what way exactly a Map would help here? How do you suggest the OP should use it? –  Simon André Forsberg Mar 17 at 20:35
4  
Just, you know, leaving this here... xkcd.com/832 –  Mooing Duck Mar 18 at 0:17
show 7 more comments

5 Answers 5

Yes, you really need to use loops and generalize this!

In order for you to learn the most, I won't give any exact code but I will tell you a bit about what you need to do

  • Your array seems to be index 1-9 based, I would recommend using index 0 to 8 instead. Array index always start at zero, it just seem like you're just not using it. Instead, use it.

  • Use for-loops to iterate through rows, columns and the diagonals of your game board. 3 rows, 3 columns, and 2 diagonals. for (int x = 0; x < 3; x++) is a simple way of looping through all x's (columns).

  • Hint: The row and column of a button can be calculated by using either index / 3 or index % 3 (% is the 'modulo'-operator). This is a reason for why you should use zero-index based arrays. It makes it so much easier to do this calculation.

  • You might want to use an enum for UNPLAYED, X and O. Using an appropriate enum can really help clean up your code massively.

    public enum PlayedBy { UNPLAYED, X, O; }

  • Decoupling. This is a bit more advanced and nothing you should be focusing on primarily. But in the future, it will help you greatly. Right now your AI knows everything about your view (view = the objects that the user sees). What if you wanted to make two AIs play against each other and only output to the console? What if you wanted to make an Android app of this? What if you wanted to make a Web application with GWT? (In the future you might want this!) You should look into the MVC pattern and/or use interfaces! to decouple your code.

You repeat this part very often

buttons[x].setText("O");
buttons[x].setEnabled(false);

That is something that should be put into a method:

void setButtonPlayer(JButton position, String player) {
    position.setText(player);
    position.setEnabled(false);
}

Now you only have to use setButtonPlayer(buttons[4], "O"); to make a move on button 4. If you use an enum, you could use setButtonPlayer(JButton button, PlayedBy player).

Once you have cleaned this up a bit, I recommend you post a follow-up question, as there is likely more things that can be cleaned up then.

share|improve this answer
    
hi, I am unsure how I can use a for loop to check all the different possibilities of the computer making the winning move. I have attempted something but i am not sure if this is correct. I have updated my post, please let me know if i am going in the correct path. thanks –  Seeker Mar 17 at 20:43
    
@Seeker Almost on the correct path. Only loop through 0 to 2, and get the index of the first button to check by i * 3. Alternatively, use a two-dimensional array JButton[][] buttons = new JButton[3][3]; Two-dimensional arrays might be difficult to get started with, but in the long run they will help you. But if you'd like, just stick to a one-dimensional array as you have now. –  Simon André Forsberg Mar 17 at 21:04
    
This question might sound a bit stupid, but why are we doing i * 3. sorry if this is silly question. –  Seeker Mar 17 at 21:06
    
@Seeker Not at all. Very good question. Think about which indexes you want to check, as I understand it you'd want (0 1 2), (3 4 5), (6 7 8). That is, (i*3, i*3 + 1, i*3 + 2). –  Simon André Forsberg Mar 17 at 21:17
1  
@Mat'sMug Yeah yeah yeah ;) –  Simon André Forsberg Mar 18 at 8:12
show 5 more comments

When confronted with a problem like this it helps to implement the chain-of-responsibility pattern. In this case, you have a hierarchy of sorts, with a winning move, defensive move, and a random move.

You should create an interface along the lines of:

public interface TicTacToeStrategy {
    public boolean apply(Button[] buttons);
}

Then, you should have a number of implementations of this strategy....

Something like:

public ForTheWinStrategy implements TicTacToeStrategy {
    public boolean apply(Button[] buttons) {
        if (.... there is a winning move) {
            apply the winning move
            return true;
        }
        return false;
    }
}

Then you can have the DefensiveStrategy, and the RandomStrategy.

 private static final TicTacToeStrategy[] STRATEGIES = {
     new ForTheWinStrategy(),
     new DefensiveStrategy(),
     new RandomStrategy()
 };

Your main code can then become:

for(TicTacToeStrategy strategy : STRATEGIES) {
    if (strategy.apply(buttons)) {
        break;
    }
}

As for all the if statements.... you need to rationalize them somehow.

Extracting them to a function would be good, or setting up a number of three-in-a-row sets....

private static final int[][] THREEINAROW = {
    {1, 2, 3},
    {4, 5, 6},
    {7, 8, 9},
    {1, 4, 7},
    {2, 5, 8},
    {3, 5, 9},
    {1, 5, 9},
    {3, 5, 7}
}

With the above setup, you can relatively easily say:

for (int[] tiaw : THREEINAROW) {
    if (2 are us, and one is empty) {
        we have a winner!
    }
} 
share|improve this answer
    
Hi, i just had a good read through on your example but i am unclear as to where new DefensiveStrategy() and new RandomStrategy() are being called from. –  Seeker Mar 17 at 20:32
    
@Seeker They are being declared as instances in the STRATEGIES array, and then they are being called inside the main loop if (strategy.apply(buttons)) {....} –  rolfl Mar 17 at 20:33
    
I was just over your code and i am finding it little confusing to understand how the last for loop works, for (int[] tiaw : THREEINAROW) {. if i am not mistaken, it loops through the pre-defined Threeinarow array and check if any two buttons have the same value and if it does, then make the winning move but how would you pass the buttons in the if statement? –  Seeker Mar 17 at 21:02
add comment

You basically hard-coded everything. It is not recommended to hard-code everything for AI. What if you were writing something a bit more complex? Then you might get a case where your AI completely fails. This is why I believe you should develop your own algorithm or implement someone else's.

For your problem, I suggest you implementing minimax algorithm which is basically recursively checking all possible options to finish the game. Here's the pseudocode:

function minimax(node, depth, maximizingPlayer)
    if depth = 0 or node is a terminal node
        return the heuristic value of node
    if maximizingPlayer
        bestValue := -∞
        for each child of node
            val := minimax(child, depth - 1, FALSE))
            bestValue := max(bestValue, val);
        return bestValue
    else
        bestValue := +∞
        for each child of node
            val := minimax(child, depth - 1, TRUE))
            bestValue := min(bestValue, val);
        return bestValue

(* Initial call for maximizing player *)
minimax(origin, depth, TRUE)

source: http://en.wikipedia.org/wiki/Minimax

Also another source that implements minimax for tic tac toe: http://www.codeproject.com/Articles/43622/Solve-Tic-Tac-Toe-with-the-MiniMax-algorithm

share|improve this answer
5  
I'm not aware of any definition of AI that excludes hard-coding all the options. –  David Richerby Mar 18 at 2:51
    
Anyone care to explain why did I get minuses? –  Sarp Kaya Mar 18 at 8:44
2  
"What if you were writing something a bit more complex? " - if the problem we're different, then yes, you'd need different code. For this problem, this approach works. –  AakashM Mar 18 at 10:58
3  
@AakashM The purpose of Code review is to review the code. This is not stackoverflow. Therefore I don't see any reason why not to recommend completely different approach, as it is more elegant and understandable. –  Sarp Kaya Mar 18 at 11:47
    
FWIW, minimax was the first thing I thought of when I read the question title and was surprised to find a string of if-else-if statements. They are fine as a start, of course, and minimax may be too advanced at the OP's stage in learning, but this answer hardly deserves a downvote! –  David Harkness Mar 18 at 16:50
add comment

I recently wrote a program to do something like this, and my implementation came down basically to this:

  • To store the marks, I had an enum Mark { NOBODY, X, O }
  • To store a move in the game, I had a Move class that tracked a mark and a location on a board
  • To store the game state, I had a Board class. My implementation made the board object immutable, and one could only derive other board objects via its put(Move m) method. It also supplied methods for querying the values held in its cells.
  • Players were represented by the Player interface. For the real player, I had a MCV thing, while my implementation of ComputerPlayer delegated to some Strategy objects. (The Player interface also had a method to report the end state of the game, to facilitate self-learning AI). On the player's turn, the game model called the player's getMove(Board b) method to obtain a Move object representing their response to that particular state.
  • However, all the information about the actual rules of the game, with regards to valid moves, winning, and the like, was contained in a separate class, Rules. One could get an Iterable containing all rows, columns, and diagonals of a board with one method, query the winner of a particular board state with another method, check if a Move object could legally be added to a Board with yet another method, or get a list of all moves they could legally make. My implementation of an AI used these exclusively for deciding where to place its move, making them fairly independent of the particular rules, and so they would still work if I changed those rules.
share|improve this answer
add comment

The problem you have is that you are checking for each specific situation, rather than abstracting for the type of situation. This makes the code you are writing much harder to read, write, debug and maintain.

So for example you have :

else if(buttons[1].getText().equals("X") && buttons[4].getText().equals("X") &&  buttons[7].getText().equals("")){
    buttons[7].setText("O");
    buttons[7].setEnabled(false);}

The first level of abstraction you could use would be to define a method to uncover what is in the square, this would make the code more readable:

private boolean containsX(int square){
    return buttons[square].getText().equals("X");}

This (and similar methods) makes the code more readable:

else if(containsX(1) && containsX(4) && isEmpty(7){
    placeMove("O", 7);}    

Now you have abstracted the code into methods you can read, however the 'logic' is still specified one piece at a time, and you can abstract that too. Other people have suggested different ways of doing it on this page, but the ideas are always the same, you need to express in general terms what each specific line is trying to do. so instead of the line above, you might have:

else if (canWinInFirstColumn)
    { playMoveInFirstColumn() }

however as soon as you start to write these, you realise that they are the same for each column, and the code for canWinInFirstColumn could simply be remade as canWinInColumnNumber(int colNumber) and would work for every column.

Then you realise that the code for columns and rows is similar, and you only really need to worry about the diagonals, so you make methods for each of those and then have a method that calls them all;

private boolean canWin(){
    for (int i=0;i<3;i++){
         if (canWinInRow(i)){
                 playMoveInRow(i);
                 return true;
                 }
         if (canWinInColumn(i)){
                 playMoveInColumn(i);
                 return true;
                 }
         }
    if (canWinInFirstDiagonal){
        playMoveInFirstDiagonal();
        return true;
        }
    if (canWinInSecondDiagonal){
        playMoveInSecondDiagonal();
        return true;
        }
    return false;
}

Obviously you can keep on going and abstract much of the code into methods that contain the logic, and experienced programmers will skip many of these steps and end up with a single method that calls a few functions. There are other 'tricks' to making the code neater, for example noticing that the board is symmetrical or using maths to work out if there is a row or column with two of your pieces and an empty space, or two of their pieces and an empty space.

Finally you have a 'view' of the board (that is buttons with the letter X is buttons with the letter X or O) assigned to them, but that is also doubling as your 'model', that is you are using it to work out what is where. It is very common to separate the two, so you would have a simple array (e.g. integers) that contains what is in each square (0=empty, 1=X, 2=O) and then your logic can be much neater (no need for .getText().equals('X') everywhere, just ==1) and you just have a single method that reads in all the 0s, 1s and 2s from the model and sets the values of the appropriate squares to X or O as appropriate whenever something changes. This means that the code that is 'intensive' is using an efficient data structure, and the code that is simply rendering the outcome (the buttons with text on them) is only being updated when it needs to be.

Hope this helps :)

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.