3
\$\begingroup\$

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("");         
    }       

}
public Boolean isClose(String me, String al)
{
    for(int i=0;i<9;i++)
    {
        if(i%3==0)
        {
            if(gui.buttons[i].getText()==gui.buttons[i+1].getText()&&gui.buttons[i].getText()==me&&gui.buttons[i+2].getText()=="")
            {
                gui.buttons[i+2].setText(al);
                gui.buttons[i+2].setEnabled(false);
                return true;
            }
            if(gui.buttons[i+1].getText()==gui.buttons[i+2].getText()&&gui.buttons[i+1].getText()==me&&gui.buttons[i].getText()=="")
            {
                gui.buttons[i].setText(al);
                gui.buttons[i].setEnabled(false);
                return true;
            }
            if(gui.buttons[i].getText()==gui.buttons[i+2].getText()&&gui.buttons[i].getText()==me&&gui.buttons[i+1].getText()=="")
            {
                gui.buttons[i+1].setText(al);
                gui.buttons[i+1].setEnabled(false);
                return true;
            }
        }
        if(i<=2)
        {
            if(gui.buttons[i].getText()==gui.buttons[i+3].getText()&&gui.buttons[i].getText()==me&&gui.buttons[i+6].getText()=="")
            {
                gui.buttons[i+6].setText(al);
                gui.buttons[i+6].setEnabled(false);
                return true;
            }
            if(gui.buttons[i+3].getText()==gui.buttons[i+6].getText()&&gui.buttons[i+3].getText()==me&&gui.buttons[i].getText()=="")
            {
                gui.buttons[i].setText(al);
                gui.buttons[i].setEnabled(false);
                return true;
            }
            if(gui.buttons[i].getText()==gui.buttons[i+6].getText()&&gui.buttons[i].getText()==me&&gui.buttons[i+3].getText()=="")
            {
                gui.buttons[i+3].setText(al);
                gui.buttons[i+3].setEnabled(false);
                return true;
            }
        }
        if(i==0)
        {
            if(gui.buttons[i].getText()==gui.buttons[i+4].getText()&&gui.buttons[i].getText()==me&&gui.buttons[i+8].getText()=="")
            {
                gui.buttons[i+8].setText(al);
                gui.buttons[i+8].setEnabled(false);
                return true;
            }   
            if(gui.buttons[i+4].getText()==gui.buttons[i+8].getText()&&gui.buttons[i+4].getText()==me&&gui.buttons[i].getText()=="")
            {
                gui.buttons[i].setText(al);
                gui.buttons[i].setEnabled(false);
                return true;
            }
            if(gui.buttons[i].getText()==gui.buttons[i+8].getText()&&gui.buttons[i].getText()==me&&gui.buttons[i+4].getText()=="")
            {
                gui.buttons[i+4].setText(al);
                gui.buttons[i+4].setEnabled(false);
                return true;
            }
        }
        if(i==2)
        {
            if(gui.buttons[i].getText()==gui.buttons[i+2].getText()&&gui.buttons[i].getText()==me&&gui.buttons[i+4].getText()=="")
            {
                gui.buttons[i+4].setText(al);
                gui.buttons[i+4].setEnabled(false);
                return true;
            }
            if(gui.buttons[i+2].getText()==gui.buttons[i+4].getText()&&gui.buttons[i+2].getText()==me&&gui.buttons[i].getText()=="")
            {
                gui.buttons[i].setText(al);
                gui.buttons[i].setEnabled(false);
                return true;
            }   
            if(gui.buttons[i].getText()==gui.buttons[i+4].getText()&&gui.buttons[i].getText()==me&&gui.buttons[i+2].getText()=="")
            {
                gui.buttons[i+2].setText(al);
                gui.buttons[i+2].setEnabled(false);
                return true;

            }   
        }                   
    }
    return false;
}
public void rand(String al)
{
    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);           
    }


}
public Boolean isFull()
{
    for(int i=0;i<9;i++)
    {
        if(gui.buttons[i].getText()=="")
            return false;
    }
    return true;
}

public boolean lineWin(String player, int i, int j, int k) {
    return gui.buttons[i].getText().equals(player) && gui.buttons[j].getText().equals(player) && gui.buttons[k].getText().equals(player);
}
public void finish(int i, int j, int k)
{
    gui.buttons[i].setBorder(BorderFactory.createBevelBorder(1, Color.CYAN, Color.CYAN));
    gui.buttons[j].setBorder(BorderFactory.createBevelBorder(1, Color.CYAN, Color.CYAN));
    gui.buttons[k].setBorder(BorderFactory.createBevelBorder(1, Color.CYAN, Color.CYAN));

    for(int m=0;m<9;m++)
    {
        gui.buttons[m].setEnabled(false);
    }


}

public Boolean checkWin(String chr){
    if(lineWin(chr,0,1,2))
    {
        finish(0, 1, 2);
        return true;
    }
    if(lineWin(chr,3,4,5))
    {
        finish(3, 4, 5);  
        return true;
    }
    if(lineWin(chr,6,7,8))
    {
        finish(6, 7, 8);
        return true;
    }
    if(lineWin(chr,0,3,6))
    {
        finish(0,3,6);
        return true;
    }
    if(lineWin(chr,1,4,7))
    {
        finish(1,4,7); 
        return true;
    }
    if(lineWin(chr,0,1,2))
    {
        finish(0, 1, 2);
        return true;
    }
    if(lineWin(chr,2,5,8))
    {
        finish(2,5,8);
        return true;
    }
    if(lineWin(chr,0,4,8))
    {
        finish(0,4,8);
        return true;
    }
    if(lineWin(chr,2,4,6))
    {
        finish(2,4,6);
        return true;
    }
    return false; }}
\$\endgroup\$
0

1 Answer 1

3
\$\begingroup\$

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 to isFull and possibly more.

An alternative

But we can actually do better if we do the check differently.

    List<Integer> openSquares = new ArrayList<>();
    for (int i = 0; i < gui.buttons.length; i++) {
        if (gui.buttons[i].getText() == "") {
            openSquares.add(i);
        }
    }

    if (openSquares.isEmpty()) {
        return;
    }

    JButton button = gui.buttons[openSquares.get(random.nextInt(openSquares.size())];

    button.setText(al);
    button.setEnabled(false);

The isEmpty check has the same result as the isFull check. So the effect of this is to do nothing and return if there are no empty squares because the board is full. Otherwise, it selects a random open square and marks it. Note that this only ever generates one random value. The original code would keep generating random values until it found one that worked. This code is probably slower on average, but its worst case is better.

DRY 2

You have eight blocks of code in this pattern:

    if(lineWin(chr,0,1,2))
    {
        finish(0, 1, 2);
        return true;
    }

Note that you pass the same three parameters to two separate methods. Consider rewriting this to something like

    int[] line = {0, 1, 2};
    if (lineWin(chr, line)) {
        finish(line);
        return true;
    }

With lineWin and finish rewritten like

public boolean lineWin(String player, int[] line) {
    for (int square : line)
    {
        if (!gui.buttons[square].getText().equals(player))
        {
            return false
        }
    }

    return true;
}

and

public void finish(int[] line)
{
    for (int square : line)
    {
        gui.buttons[square].setBorder(BorderFactory.createBevelBorder(1, Color.CYAN, Color.CYAN));
    }

    for (JButton button : gui.buttons)
    {
        button.setEnabled(false);
    }
}

This ensures that you'll pass the same values to both methods.

Also, this allows for line lengths other than three squares.

Or something like

    for (Integer[] line : lines) {
        if (lineWin(player, line)) {
            finish(line);
            return true;
        }
    }

    return false;

Where lines may be defined something like

private static final List<Integer[]> lines = new ArrayList<>();

static {
    lines.add(new Integer[]{0, 1, 2});
    lines.add(new Integer[]{3, 4, 5});
    lines.add(new Integer[]{6, 7, 8});

    lines.add(new Integer[]{0, 3, 6});
    lines.add(new Integer[]{1, 4, 7});
    lines.add(new Integer[]{2, 5, 8});

    lines.add(new Integer[]{0, 4, 8});
    lines.add(new Integer[]{2, 4, 6});
}

This is only done once for the lifetime of the application.

Or something like

public class Board {

    private final String[] squares;
    private final List<Integer[]> lines = new ArrayList<>();

    Board(int width, int height) {
        squares = new String[width * height];
        for (int column = 0; column < width; column++) {
            lines.add(makeColumn(column, height, width));
        }

        for (int row = 0; row < squares.length; row += width) {
            lines.add(makeRow(row, width));
        }

        int rightEdge = width - 1;
        if (width >= height) {
            int difference = width - height + 1;
            for (int diagonal = 0; diagonal < difference; diagonal++) {
                lines.add(makeForwardDiagonal(0, diagonal, width, height));
                lines.add(makeBackwardDiagonal(0, rightEdge - diagonal, width, height));
            }
        } else {
            int difference = height - width + 1;
            for (int diagonal = 0; diagonal < difference; diagonal++) {
                lines.add(makeForwardDiagonal(diagonal, 0, width, width));
                lines.add(makeBackwardDiagonal(diagonal, rightEdge, width, width));
            }
        }
    }

    private Integer[] makeBackwardDiagonal(int row, int column, int width, int length) {
        Integer[] line = new Integer[length];
        for (int i = 0; i < length; i++) {
            line[i] = (row + i) * width + column - i;
        }

        return line;
    }

    private Integer[] makeForwardDiagonal(int row, int column, int width, int length) {
        Integer[] line = new Integer[length];
        for (int i = 0; i < length; i++) {
            line[i] = (row + i) * width + column + i;
        }

        return line;
    }

    private Integer[] makeRow(int row, int width) {
        Integer[] line = new Integer[width];
        for (int i = 0; i < width; i++) {
            line[i] = row + i;
        }

        return line;
    }

    private Integer[] makeColumn(int column, int height, int width) {
        Integer[] line = new Integer[height];
        for (int i = 0; i < height; i++) {
            line[i] = i * width + column;
        }

        return line;
    }

}

Which you can demonstrate by adding

    public static void main(String[] args) {
        Board board = new Board(5, 3);
        for (Integer[] line : board.lines) {
            System.out.println(Arrays.toString(line));
        }
    }

I'll leave it up to you to integrate it into your application if desired. Note that some of the relevant changes would happen outside the class that you posted.

\$\endgroup\$
3
  • \$\begingroup\$ Alright, there are just 2 things that I cant get along with. 1) the alternative way for the isFull() method. JButton button = gui.buttons[random.nextInt(openSquares.size()]; random.nextInt(openSquares.size())]; If I have 3 buttons open for examples, buttons[5], buttons[0] and buttons[2], this will make openSquares.size()=3, and generates a number between 0-3.But what actually should happen is a number generated according to the values that are inside the ArrayList(5-0-2). \$\endgroup\$
    – Heshamy
    Oct 23, 2016 at 11:11
  • \$\begingroup\$ Cant edit or delete my comment for some reason. 2) int[] line = {0, 1, 2}; if(lineWin(chr,line)) this doesn't work, it says that (int, int, int) isnt applicable for the argument(int[]) Everything else is great, thanks alot. \$\endgroup\$
    – Heshamy
    Oct 23, 2016 at 11:13
  • \$\begingroup\$ @Heshamy I fixed the error in the the random selection and wrote the missing method changes. \$\endgroup\$
    – mdfst13
    Oct 23, 2016 at 17:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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