Tic Tac Toe against Al

I'm working on a project that has an option to playing against Al, so I started to make it in a separate class and just extended the main class which has an object of a GUI class which draws the buttons and frames.

My algorithm works as follows:

Once a button is clicked:

• Set the text to "X"

• Check if player "X"(human) wins. If yes, that's it. If not:

• Check if there's a winning move for player "O"(Computer). If yes, do it. If not:

• Check if there's a winning move for player "X"(Human). If yes, block it. If not:

• Choose a random/empty place and set its text to "O"

• Check if player "O"(Computer) wins

I'm wondering if there's any updates/changes I should do to improve my code, either in the algorithm or the code itself.

```
public class Al extends main implements ActionListener{
int turn=0;
public void actionPerformed(ActionEvent e)
{
JButton temp=(JButton)e.getSource();
temp.setEnabled(false);

temp.setText(turn%2==0?"X":"O");
if(checkWin(turn%2==0?"X":"O"))
gui.banner.setText("You won!");
else{
if(isClose(turn%2==0?"O":"X",turn%2==0?"O":"X")==false && isClose(turn%2==0?"X":"O", turn%2==0?"O":"X")==false)
rand(turn%2==0?"O":"X");
if(checkWin(turn%2==0?"O":"X"))
gui.banner.setText("You Lost!");
}

}
Al()
{
gui.addbanner("Your Turn");
gui.addnewbutton();
gui.newgame.addActionListener(new ActionListener() {
public void actionPerformed(ActionEvent arg0) {
for(int i = 0;i < 9 ; i++){
gui.buttons[i].setEnabled(true);
gui.buttons[i].setBorder(BorderFactory.createBevelBorder(1,Color.black,Color.black));
gui.buttons[i].setText("");

}
gui.banner.setText("Your Turn");
turn++;
if(turn%2==1)
rand("O");
}
});
for(int i=0;i<9;i++){
gui.buttons[i].addActionListener(this);
gui.buttons[i].setText("");

Untested

I haven't tried these changes, as your code is not runnable without more context. If you'd provided the other classes, I could have run them and seen what happened. Then I could make changes in my local IDE and test them (at least minimally). As is, I can't even guarantee that the changes will compile.

For small programs like this, consider including all the code in the question. For larger programs, consider linking to external repositories with more context.

Don't Repeat Yourself (DRY)

temp.setText(turn%2==0?"X":"O");
    if(checkWin(turn%2==0?"X":"O"))
        gui.banner.setText("You won!");
    else{
        if(isClose(turn%2==0?"O":"X",turn%2==0?"O":"X")==false && isClose(turn%2==0?"X":"O", turn%2==0?"O":"X")==false)
            rand(turn%2==0?"O":"X");
        if(checkWin(turn%2==0?"O":"X"))
            gui.banner.setText("You Lost!");
    }

You calculate the current and other player multiple times with parallel logic. Instead consider

Player current = (turn % 2 == 0) ? Player.X : Player.O;
    Player other = current.getNext();
    temp.setText(current.toString());
    if (checkWin(current)) {
        gui.banner.setText("You won!");
    } else {
        if (!isClose(other, current) && !isClose(current, other)) {
            rand(other);
        }

        if (checkWin(other))
            gui.banner.setText("You Lost!");
        }
    }

Note that this also requires a Player enum with a getNext method defined manually. In this case, getNext would return the opposite of current. So if current were X, it would return O and vice versa.

You might ask why you couldn't just do

String other = (turn%2 == 0) ? "O" : "X";

The short answer is that you could do that but may not want to do so. The longer answer is that that is fragile. For example, if you change things so that O goes first and X second, you have to change every occurrence in your original code. And in the new code, you have to change two places. If instead you base the second result on the first result, both move together. Moving it into the enum makes it more likely that you'll make any needed changes at once. For example, if you decided that you wanted the letters to be G and T.

For a similar reason, it is better to use an enum, which will complain if you happen to type a Y where you meant to say X. The current code will silently not work right in that case and you'd have to do runtime testing to determine why. The revised code would give a compile time error instead.

It is more idiomatic to write !isClose(current, other) than comparing it to false. Both do the same thing, but the former is the more common way of writing it.

I prefer always using the block form with the curly braces even when the single statement form would work. I find it more consistent and easier to read. Plus, the single statement form is vulnerable to a certain type of editing error that the block form is not.

Avoid magic numbers

for(int i=0;i<9;i++){
        gui.buttons[i].addActionListener(this);
        gui.buttons[i].setText("");         
    }

You use this pattern a number of times. The most obvious fix is a constant, which you'd use like

for (int i = 0; i < BUTTONS_COUNT; i++){
        gui.buttons[i].addActionListener(this);
        gui.buttons[i].setText("");         
    }

Then all of them would move together. But we actually have an even better solution here, which is simpler and saves defining an extra constant:

for (JButton button : gui.buttons) {
        button.addActionListener(this);
        button.setText("");         
    }

Now we don't declare an iteration variable that we only use to dereference an array. We work on the contents of the array directly. We only work with those that are there. And if we change the number of squares in the board, this adjusts automatically. We can never increase or decrease the number of buttons in the board but forget a place where we interacted manually.

Don't make multiple Random objects

Random random=new Random();

If you make this into a static final variable on the class, then you'll only have to create and seed it once.

public static final Random random = new Random();

Don't duplicate work

We can actually simplify the entire method.

int x;
    Random random=new Random();

    do
    {
        x=random.nextInt(9)-0;
    }
    while(gui.buttons[x].getText()!="" && isFull()==false);
    if(isFull()==false)
    {
        gui.buttons[x].setText(al);
        gui.buttons[x].setEnabled(false);           
    }

The isFull method will return the same result each time. So we can do that first.

if (isFull())
    {
        return;
    }

    int x;
    do
    {
        x = random.nextInt(9)-0;
    }
    while (gui.buttons[x].getText() != "");

    gui.buttons[x].setText(al);
    gui.buttons[x].setEnabled(false);

This saves at least one call t

StackExchange Code Review Q#144892, answer score: 3