1
\$\begingroup\$

This code works so far and it is a TicTacToe game (Personal Project of Mine). I'm kinda new to programming. My question is I really do not like how the Connected method is at all, because well it's fairly large for no reason. Is there a way to make it more compact because I have to add the middle and bottom row then do the vertical columns then do the diagonal lines and that will make method very big so what do you guys think?

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

/**
 * Created by Usman on 5/17/2016.
 */
public class TicTacToeGUI extends JPanel {

    boolean turn = true;
    JButton[] board;

    public TicTacToeGUI() {
        setLayout(new GridLayout(3,3));

        board = new JButton[9];
        for (int i = 0; i < board.length; i++) {
            board[i] = new JButton("");
            board[i].setPreferredSize(new Dimension(70, 60));
            board[i].setFont(new Font("Arial", Font.PLAIN, 40));
            int finalI = i;
            board[i].addActionListener(new ActionListener() {
                @Override
                public void actionPerformed(ActionEvent e) {

                    if(board[finalI].isEnabled()){
                        if(turn){
                            board[finalI].setText("X");
                            turn = false;
                        }
                        else{
                            board[finalI].setText("O");
                            turn = true;
                        }
                        board[finalI].setEnabled(false);
                    }
                    Connected();
                }
            });
            add(board[i]);
        }
    }
    public void Connected(){
        //Horizontal top row
        if((!board[0].isEnabled()) || (!board[1].isEnabled()) || (!board[2].isEnabled())){
            if((board[0].getText() == board[1].getText()) && (board[0].getText() == board[2].getText())){
                if((!board[0].isEnabled()) == (!board[1].isEnabled()) == (!board[2].isEnabled())){
                    JOptionPane.showMessageDialog(null, "You've Won The Game!");
                }
            }
        }
    }
    public static void main(String []args){
        TicTacToeGUI game = new TicTacToeGUI();
        JFrame frame = new JFrame("Tic Tac Toe");
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.getContentPane().add(game);
        frame.pack();
        frame.setVisible(true);
    }
}
\$\endgroup\$
1
\$\begingroup\$

You're checking if each section of your board is enabled as part of your check, this check makes your test look more complicated than it needs to be. Part of this is because your board is tightly integrated into its graphic representation. Generally speaking you want to try to avoid this, it makes both layers easier to change.

Rather than having a signature of Connected() consider writing something more like this (pseudo code):

bool CheckWin(string currentPlayer) {
    foreach row in rows
        return true if CheckWinningRow(row, currentPlayer)
    // Check Columns
    // Check Diagonals
}

bool CheckWinningRow(int row, string currentPlayer) {
    return true if (board[row*3] == currentPlayer &&
                    board[row*3+1] == currentPlayer &&
                    board[row*3+2] == currentPlayer);
}

Checking each row is going to be the same action, it's just going to be a different starting location in your array.

You only need to check if the last player (currentPlayer, either X or O) has won. If you work by checking the current player, you don't need to know if the button is enabled (has already been played) it's not going to have the current player's value in it if it hasn't been played on yet.

\$\endgroup\$
3
  • \$\begingroup\$ Do you mind rephrasing " Part of this is because your board is tightly integrated into its graphic representation. Generally speaking you want to try to avoid this, it makes both layers easier to change." I did not really understand it. The only layer I have are made up of JButtons \$\endgroup\$ – Usman Bashiru May 19 '16 at 17:24
  • \$\begingroup\$ @UsmanBashiru As applications get bigger you start thinking about them in layers, presentation layer for user interaction, logic layer where the guts of the application sits, data layer for accessing files/databases. At the moment you only have one layer which means that when you think about how to check for victory in addition to thinking about what victory means you also have to think about the elements that make up the interface and interact with the user, like you JButtons. \$\endgroup\$ – forsvarir May 19 '16 at 17:29
  • \$\begingroup\$ Oh I get it so basically I have to break up my code (methods) into smaller tasks instead of a few big task? I think I've heard something like that before called the OSI model. Thank you for clearing this up for me \$\endgroup\$ – Usman Bashiru May 19 '16 at 17:47

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.