7
\$\begingroup\$

This is just a simple PvP tic tac toe program, I'm learning and just looking for maybe some feedback on how to improve it?

There are mainly two things I can see improvement for, the action listener where I could do something like; "a button has been pressed, get the name of the button, pass the name of the button, and change button to X or O based on that". It would shorten the lines from needing one for each button, to just one for the whole button array.

And also the gameWin check might be able to be improved? I don't know, maybe like the tie or using another way to compare ordered numbers in an array?

I'm not experienced and simply doing this for fun, any feedback would be awesome XD

package ttt;

import javax.swing.*;
import java.awt.*;
import java.awt.event.*;

public class Ttt {

    static JFrame mainFrame = new JFrame("Tic Tac Toe");
    static JButton[] gameButtons = new JButton[9];

    public static void main (String args[]) {
        getGui();
    }

    public static void getGui() {
        mainFrame.setVisible(true);
        mainFrame.setResizable(false);
        mainFrame.setSize(600, 620);
        mainFrame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        JPanel mainPanel = new JPanel(new GridLayout(3, 3));
        mainFrame.add(mainPanel);
        for (int i = 0; i < gameButtons.length; i++) {
            gameButtons[i] = new JButton(String.valueOf(i));
            gameButtons[i].setForeground(Color.WHITE);
            gameButtons[i].setBackground(Color.WHITE);
            gameButtons[i].setFont(new Font("Times New Roman", Font.BOLD, 60));
            gameButtons[i].addActionListener(new Response());
            mainPanel.add(gameButtons[i]);
        }
    }

    private static class Response implements ActionListener {

        static int rounds = 0;
        static JButton winMessage = new JButton(), tieMessage = new JButton();

        @Override
        public void actionPerformed(ActionEvent e) {
            if (e.getSource() == gameButtons[0]) {
                roundJudge(0);
            } else if (e.getSource() == gameButtons[1]) {
                roundJudge(1);
            } else if (e.getSource() == gameButtons[2]) {
                roundJudge(2);
            } else if (e.getSource() == gameButtons[3]) {
                roundJudge(3);
            } else if (e.getSource() == gameButtons[4]) {
                roundJudge(4);
            } else if (e.getSource() == gameButtons[5]) {
                roundJudge(5);
            } else if (e.getSource() == gameButtons[6]) {
                roundJudge(6);
            } else if (e.getSource() == gameButtons[7]) {
                roundJudge(7);
            } else if (e.getSource() == gameButtons[8]) {
                roundJudge(8);
            } else if (e.getSource() == winMessage) {
                System.exit(0);
            } else if (e.getSource() == tieMessage) {
                System.exit(0);
            }
            gameJudge();
        }

        public void roundJudge(int buttonNumber) {
            if (rounds %2 == 0) {
                gameButtons[buttonNumber].setText("X");
            } else if (rounds %2 == 1) {
                gameButtons[buttonNumber].setText("O");
            }
            gameButtons[buttonNumber].setEnabled(false);
            rounds += 1;
        }

        public void gameJudge() {
            if (gameButtons[0].getText().equals(gameButtons[1].getText()) && gameButtons[1].getText().equals(gameButtons[2].getText())) {
                gameWin(gameButtons[0].getText());
            } else if (gameButtons[3].getText().equals(gameButtons[4].getText()) && gameButtons[4].getText().equals(gameButtons[5].getText())) {
                gameWin(gameButtons[3].getText());
            } else if (gameButtons[6].getText().equals(gameButtons[7].getText()) && gameButtons[7].getText().equals(gameButtons[8].getText())) {
                gameWin(gameButtons[6].getText());
            } else if (gameButtons[0].getText().equals(gameButtons[3].getText()) && gameButtons[3].getText().equals(gameButtons[6].getText())) {
                gameWin(gameButtons[0].getText());
            } else if (gameButtons[1].getText().equals(gameButtons[4].getText()) && gameButtons[4].getText().equals(gameButtons[7].getText())) {
                gameWin(gameButtons[1].getText());
            } else if (gameButtons[2].getText().equals(gameButtons[5].getText()) && gameButtons[5].getText().equals(gameButtons[8].getText())) {
                gameWin(gameButtons[2].getText());
            } else if (gameButtons[0].getText().equals(gameButtons[4].getText()) && gameButtons[4].getText().equals(gameButtons[8].getText())) {
                gameWin(gameButtons[0].getText());
            } else if (gameButtons[2].getText().equals(gameButtons[4].getText()) && gameButtons[4].getText().equals(gameButtons[6].getText())) {
                gameWin(gameButtons[2].getText());
            } else if (gameButtons[0].isEnabled() == false && gameButtons[1].isEnabled() == false && gameButtons[2].isEnabled() == false && gameButtons[3].isEnabled() == false && gameButtons[4].isEnabled() == false && gameButtons[5].isEnabled() == false && gameButtons[6].isEnabled() == false && gameButtons[7].isEnabled() == false && gameButtons[8].isEnabled() == false) {
                gameTie();
            }
        }

        public static void endFormat(JFrame name1, JButton name2) {
            for(JButton gameButton : gameButtons) {
                gameButton.setVisible(false);
                gameButton.setEnabled(false);
            }
            mainFrame.setVisible(false);
            name1.setLayout(new GridLayout(1, 1));
            name1.setVisible(true);
            name1.setResizable(false);
            name1.setSize(600, 620);
            name1.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
            name2.setToolTipText("Click anywhere to exit the program.");
            name2.setFont(new Font("Times New Roman", Font.BOLD, 60));
            name2.addActionListener(new Response());
            name2.setOpaque(false);
            name2.setContentAreaFilled(false);
            name2.setBorderPainted(false);
        }

        public static void gameWin(String team) {
            JFrame winFrame = new JFrame("Tic Tac Toe");
            endFormat(winFrame, winMessage);
            winMessage.setText("Player " + team + " Won!");
            winFrame.add(winMessage);
        }

        public static void gameTie() {
            JFrame tieFrame = new JFrame("Tic Tac Toe");
            endFormat(tieFrame,tieMessage);
            tieMessage.setText("Tie!");
            tieFrame.add(tieMessage);
        }

    }

}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to codereview. As we all want to get some code reviews here you should change your title to describe what the code is about, not what you'd like to get from an answer. Every post in here might have the same title as yours. \$\endgroup\$ – яүυк Oct 4 '16 at 20:15
  • \$\begingroup\$ Rather than initialize your GUI in main you should use SwingUtilities::invokeLater. That's my 2¢. \$\endgroup\$ – CAD97 Oct 12 '16 at 2:17
  • \$\begingroup\$ @Dex'ter thanks for your comment, you didn't have to respond to a newb like me, I'll try to be more correct with my titles in the future, appreciate it man :D \$\endgroup\$ – Archie Oct 12 '16 at 16:03
  • \$\begingroup\$ @CAD97 thanks man, I still have a lot to learn and I think part of that is learning setters and getters and passing instead of calling, not directly calling my gui from main will help a bunch, appreciate it :D \$\endgroup\$ – Archie Oct 12 '16 at 16:04
4
\$\begingroup\$
  • actionPerformed

    Since you create a new Response() for each button, you may as well make it have an identification information.

    However, a dedicated per-button Response() is an overkill. You can uniquely identify each button with gameButton[i].setActionCommand("" + i);. Then e.getActionCommand() in the will tell you which button was pressed.

  • roundJudge

    Is a bit more complicated then necessary. Consider

    private string[] labels = { "X", "O" };
    private void roundJudge(int i) {
        gameButton[i].setText(labels[round % 2]);
        gameButton[i].setEnabled(false);
        round++;
    }
    
\$\endgroup\$
  • \$\begingroup\$ thank you so much for this, I've been trying to group it under arrays and like access which individual element was passed, but none of that works. I'll test it out within the next few days XD really really appreciate it man \$\endgroup\$ – Archie Oct 12 '16 at 16:14
  • \$\begingroup\$ ummm so I know I can use .getActionCommand() to identify the name of the button that was pressed, but how do I get the action listener to know something was pressed? So I added action listener to all the buttons, in the actionPerformed() function, how do I say if(button was pressed) { \$\endgroup\$ – Archie Oct 13 '16 at 14:09
  • \$\begingroup\$ getButtonName() and then I can use that getActionCommand to affect the button that was pressed \$\endgroup\$ – Archie Oct 13 '16 at 14:10
  • \$\begingroup\$ this is what I have right now: public void actionPerformed(ActionEvent e) { if(button was pressed) { roundJudge(e.getActionCommand()); } gameJudge(); } \$\endgroup\$ – Archie Oct 13 '16 at 14:11
4
\$\begingroup\$

Some immediate thoughts:

1. actionPerformed: I notice that you're going through the array looking for a match and calling roundJudge with the same index. This can be shortened to a for loop:

for (int i = 0; i < 9; i++) {
    if (e.getSource() == gameButtons[i]) {
        roundJudge(i);
        return;
    }
}

2. gameJudge has three segments: checking rows, checking columns, and checking diagonals. The first two can easily be turned into for loops as well:

if (gameButtons[i].getText().equals(gameButtons[i+1].getText()) && //etc.

3. (Advanced) If you are using Java 8, you can check for ties across the entire array using a Stream:

boolean allSquaresFull = Arrays.stream(gameButtons).noneMatch(JButton::isEnabled);
\$\endgroup\$
  • \$\begingroup\$ The problem with that right now is i isn't a variable in actionListener(), so if I try to call the source, it gives an error, and thanks a lot man, I'll test out the gameJudge idea within the next couple of days, really appreciate it @JoeC \$\endgroup\$ – Archie Oct 12 '16 at 16:17
2
\$\begingroup\$

Imagine you had a method haveEqualText(int index1, int index2, int index3). Then your method to check for a winner could look like this:

boolean isWon() {
  return haveEqualText(0, 3, 6)
      || haveEqualText(1, 4, 7)
      || haveEqualText(2, 5, 8)
      || …;
}
\$\endgroup\$
  • \$\begingroup\$ this is why I'm stupid... XD thank you very much I'll test it out, I'm still learning but you've just sped that up, I didn't think of having a function to do the checking, that would eliminate all that repeating code, I appreciate it :D \$\endgroup\$ – Archie Oct 12 '16 at 16:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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