15
\$\begingroup\$

I have written a simple GUI Tic Tac Toe Application and since this is my first shot, I think it can be improved a lot.

Please tell me what you think about it and what you see I made wrong so I can avoid those mistakes the next time.

Is there code redundancy I don't see? Is everything declared and implemented at its right place?

Earlier I wrote a console based game and then tried to convert it in to object oriented based. I am not sure if this can be called object oriented because I have a little knowledge about object oriented structure as I am a beginner.

Game

Main Class

package tic_tac_adv;

public class Tic_Tac_Adv {

public static void main(String[] args) {

    // Main CLass

    Control C = new Control(); 

}

}

Control Class

package tic_tac_adv;

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

public class Control extends JFrame {

/*
this is the main controler that controls
and combines different components of the game.
*/ 

private Board GameBoard; //Board and Button
private Tools TButtons; // Exit and Reset

Control() {

    setLayout(new BorderLayout());

    GameBoard = new Board();
    TButtons = new Tools();

    TButtons.SetObject(GameBoard);

    add(GameBoard, BorderLayout.CENTER);
    add(TButtons, BorderLayout.SOUTH);

    setVisible(true);
    setSize(350, 350);
    setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
}

}

Board Class

   package tic_tac_adv;

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

public class Board extends JPanel implements ActionListener {

private JButton B1, B2, B3, B4, B5, B6, B7, B8, B9; // Buttons
private GameArray GArray; // Class with Array
private boolean Player = false;
private int PlayerMark = 1;

/*
 Player is the Current players turn. if false player 1 will play else player 2
 PlayerMark is to set number to the array "1" for player 1 and "2" for player 2
 */
Board() {

    // creates the panel
    setLayout(new GridLayout(3, 3));

    B1 = new JButton("");
    B2 = new JButton("");
    B3 = new JButton("");
    B4 = new JButton("");
    B5 = new JButton("");
    B6 = new JButton("");
    B7 = new JButton("");
    B8 = new JButton("");
    B9 = new JButton("");

    SetGame();

    add(B1);
    add(B2);
    add(B3);
    add(B4);
    add(B5);
    add(B6);
    add(B7);
    add(B8);
    add(B9);

    B1.addActionListener(this);
    B2.addActionListener(this);
    B3.addActionListener(this);
    B4.addActionListener(this);
    B5.addActionListener(this);
    B6.addActionListener(this);
    B7.addActionListener(this);
    B8.addActionListener(this);
    B9.addActionListener(this);

}

public void SetGame() {

    GArray = new GameArray(this);

    DefaultText();
    DisableAll(true);


    Player = false; // default Value
    PlayerMark = 1; // default Value
}

public void Reset() {

    SetGame(); // To Reset the Game

}

public void actionPerformed(ActionEvent E) {

    JButton Pressed = (JButton) E.getSource();

    /*
     if any button is pressed the value is sent to GameArray class
     */
    if (Pressed == B1) {
        GArray.ArrayInitialize(0, 0, PlayerMark);
        SetText(Pressed, Player);     // chaneg button text to "X" or "O" based on player turn
        PlayerMark = SwithTurn(Player); // Swithch Turns
        ButtonDisabler(B1); // Disable pressed Button

    } else if (Pressed == B2) {
        GArray.ArrayInitialize(0, 1, PlayerMark);
        SetText(Pressed, Player);
        SetText(Pressed, Player);
        PlayerMark = SwithTurn(Player);
        ButtonDisabler(B2);
    } else if (Pressed == B3) {
        GArray.ArrayInitialize(0, 2, PlayerMark);
        SetText(Pressed, Player);
        PlayerMark = SwithTurn(Player);
        ButtonDisabler(B3);

    } else if (Pressed == B4) {
        GArray.ArrayInitialize(1, 0, PlayerMark);
        SetText(Pressed, Player);
        PlayerMark = SwithTurn(Player);
        ButtonDisabler(B4);

    } else if (Pressed == B5) {
        GArray.ArrayInitialize(1, 1, PlayerMark);
        SetText(Pressed, Player);
        PlayerMark = SwithTurn(Player);
        ButtonDisabler(B5);

    } else if (Pressed == B6) {
        GArray.ArrayInitialize(1, 2, PlayerMark);
        SetText(Pressed, Player);
        PlayerMark = SwithTurn(Player);
        ButtonDisabler(B6);
    } else if (Pressed == B7) {
        GArray.ArrayInitialize(2, 0, PlayerMark);
        SetText(Pressed, Player);
        PlayerMark = SwithTurn(Player);
        ButtonDisabler(B7);
    } else if (Pressed == B8) {
        GArray.ArrayInitialize(2, 1, PlayerMark);
        SetText(Pressed, Player);
        PlayerMark = SwithTurn(Player);
        ButtonDisabler(B8);
    } else if (Pressed == B9) {
        GArray.ArrayInitialize(2, 2, PlayerMark);
        SetText(Pressed, Player);
        PlayerMark = SwithTurn(Player);
        ButtonDisabler(B9);
    }

}

public int SwithTurn(boolean last) {

    // if the past player was false(player 1) then swith to true(player 2)
    System.out.println();

    if (last == true) {
        Player = false;
        return 1;

    } else if (last == false) {
        Player = true;
        return 2;
    } else {
        return 3;
    }

}

public void ButtonDisabler(JButton Btn) {

    Btn.setEnabled(false); // Disable Button

}

public void DisableAll(boolean Opp) {

    // Disable  All Buttons
    B1.setEnabled(Opp);
    B2.setEnabled(Opp);
    B3.setEnabled(Opp);
    B4.setEnabled(Opp);
    B5.setEnabled(Opp);
    B6.setEnabled(Opp);
    B7.setEnabled(Opp);
    B8.setEnabled(Opp);
    B9.setEnabled(Opp);

}

public void SetText(JButton Btn, boolean Play) {

    if (Play == true) {
        Btn.setText("O");
    } else if (Play == false) {
        Btn.setText("X");
    }

}

public void DefaultText(){

    B1.setText("");
    B2.setText("");
    B3.setText("");
    B4.setText("");
    B5.setText("");
    B6.setText("");
    B7.setText("");
    B8.setText("");
    B9.setText("");

}


}

Array Class

package tic_tac_adv;

import javax.swing.*;

public class GameArray {

/*

Class for Array

*/

private Board Brd;
private int GameArr[][];
private boolean Turn;
private JButton Pressed;

GameArray(Board B) {

    GameArr = new int[3][3];

    Brd = B;

    for (int i = 0; i < 3; i++) {
        for (int j = 0; j < 3; j++) {
            GameArr[i][j] = 0;
        }
    }

}

public void ArrayInitialize(int i, int j, int Marker) {

    //Set Data sent by Action listener in Board

    GameArr[i][j] = Marker;
    WinCheck(Marker);
}


public void WinCheck(int Marker) {

    // if the specified array indexs contain a certain Maker (1,2) on winning pattrens then announce winner

    if ((GameArr[0][0] == Marker && GameArr[0][1] == Marker && GameArr[0][2] == Marker) || (GameArr[1][0] == Marker && GameArr[1][1] == Marker && GameArr[1][2] == Marker) || (GameArr[2][0] == Marker && GameArr[2][1] == Marker && GameArr[2][2] == Marker)) {

        if (Marker == 1) {

            JOptionPane.showMessageDialog(Brd, "CONGRATULATIONS : Player 1 (Winner)");

        } else if (Marker == 2) {

            JOptionPane.showMessageDialog(Brd, "CONGRATULATIONS : Player 2 (Winner)");

        }

        Brd.DisableAll(false);

    } else if ((GameArr[0][0] == Marker && GameArr[1][0] == Marker && GameArr[2][0] == Marker) || (GameArr[0][1] == Marker && GameArr[1][1] == Marker && GameArr[2][1] == Marker) || (GameArr[0][2] == Marker && GameArr[1][2] == Marker && GameArr[2][2] == Marker)) {

        if (Marker == 1) {
            JOptionPane.showMessageDialog(Brd, "CONGRATULATIONS : Player 1 (Winner)");

        } else if (Marker == 2) {
            JOptionPane.showMessageDialog(Brd, "CONGRATULATIONS : Player 2 (Winner)");

        }

        Brd.DisableAll(false);

    } else if ((GameArr[0][0] == Marker && GameArr[1][1] == Marker && GameArr[2][2] == Marker) || (GameArr[2][0] == Marker && GameArr[1][1] == Marker && GameArr[0][2] == Marker)) {

        if (Marker == 1) {
            JOptionPane.showMessageDialog(Brd, "CONGRATULATIONS : Player 1 (Winner)");

        } else if (Marker == 2) {
            JOptionPane.showMessageDialog(Brd, "CONGRATULATIONS : Player 2 (Winner)");

        }
        Brd.DisableAll(false);

    }

}

}

Tool Class

package tic_tac_adv;

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

public class Tools extends JPanel {

private JButton Exit, Reset;
private Board Brd;

Tools() {

    setLayout(new FlowLayout());

    Exit = new JButton("Exit");
    Reset = new JButton("Reset");

    Exit.addActionListener(new ActionListener() {

        public void actionPerformed(ActionEvent ae) {
            System.exit(0);
        }
    });

    Reset.addActionListener(new ActionListener() {

        public void actionPerformed(ActionEvent ae) {

            Brd.Reset();

        }

    });

    add(Exit);
    add(Reset);
}

public void SetObject(Board B) {

    Brd = B;

}

}
\$\endgroup\$
10
\$\begingroup\$

Mathew had some great responses, but I'd like to explain a little more about using for loops. If you take a look at your Board() code right now, you might notice how a lot of the code is the same - when you make the JButtons, for example, you call new JButton("") nine times in a row. In general, having a lot of code that all looks the same or is copy-pasted is a bad idea. You can use a for loop with an array to clean this up a lot. Here's how Board() would look after using an array.

Board() {
    // creates the panel
    setLayout(new GridLayout(3, 3));

    buttons = new JButton[9];
    for (int i=0; i < buttons.length; i++) {
        buttons[i] = new JButton("");
    }

    SetGame();

    for (int i=0; i < buttons.length; i++) {
        add(buttons[i]);    
        buttons[i].addActionListener(this);
    }    
}

See how much shorter and less repetitive that is?

As a side note, by not specifying 'public' or 'private', you implicitly made your constructors package-private, which is rarely the right solution. I recommend specifying everything as either public or private, until you get to the point of distributing your code or compiled programs, in which case package-private might occasionally make sense.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the detailed information :) I had a similar idea in mind because i wanted minimum repetition. But i was not sure about the implementation :) \$\endgroup\$ – Taha Ishfaq Bhutta Jul 16 '14 at 15:37
7
\$\begingroup\$

That's some nice code! And welcome to Code Review.

  1. It's customary to name variables and methods starting with lowercase and classes starting with uppercase. This way you can easily tell which is which. So for example:

    public class Control extends JFrame { // fine as is - classes are capitalized
    

    GameBoard = new Board(); // "gameBoard" should be lowercase
    

    private JButton Exit, Reset; // "exit" and "reset" should be lowercase
    

    public class Tic_Tac_Adv { // should be "TicTacAdv"
    
  2. There's a lot of blank lines. Is that intentional?

Nit-picky stuff

  1. package tic_tac_adv;

    When you are more comfortable with Java you'll want to make a package name that NO ONE else is using. The traditional way to do this is to use a website that you or your company controls, in reverse order.

    For example:

    package com.mycompany.games.tictactoe;
    

Ok, I guess that's it. When you get more advanced you can use loops in your WinCheck method. So instead of testing eight things, you can use one loop to check all the rows, a second loop to check all the columns, and then you check both diagonals. For 3x3 Tic-Tac-Toe it doesn't really matter, but I think you can see how a loop would make things much easier if you were doing 4x4 or 5x5!

\$\endgroup\$
  • \$\begingroup\$ Thank you for your comment :) I'll keep all these tips and Suggestion in mind and make changes in WinCheck();. Thanks again :) \$\endgroup\$ – Taha Ishfaq Bhutta Jul 16 '14 at 8:13
  • \$\begingroup\$ I'm sorry, but I have to disagree with "It looks very clean". There really need to be loops for code like this, otherwise it is not clean. \$\endgroup\$ – Simon Forsberg Jul 17 '14 at 11:49
  • \$\begingroup\$ I just removed that one sentence. I don't really have the time/energy to do a whole rewrite. \$\endgroup\$ – Matthew Jacobs Jul 19 '14 at 4:20
6
\$\begingroup\$

You can also use a for-loop inside your actionPerformed method.

for (int button = 0; button <= 8; button++) {
    if (pressed == buttons[button]) {
        GArray.arrayInitialize(button / 3, button % 3, playerMark);
        setText(pressed, player);
        playerMark = switchTurn(player);
        buttonDisabler(pressed);
    }
}

An important trick here is that you can calculate the x and y positions of your buttons with a bit of math. By using button / 3 and button % 3.

Your SwithTurn method (which should be named switchTurn) is a bit ugly. It can be improved like this:

public int SwithTurn(boolean last) {
    player = !last;
    if (last) {
        return 1;
    } else {
        return 2;
    }
}

For the return, you can also use the conditional operator:

return last ? 1 : 2;
\$\endgroup\$
1
\$\begingroup\$

Also you don't need:

Control c = new Control();

You can just use:

new Control();

For this program

\$\endgroup\$
0
\$\begingroup\$

@raptortech97 Loops were a great improvement :)

   package tic_tac_adv;

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

public class Board extends JPanel implements ActionListener {

private JButton Buttons[];
private GameArray GArray; // Class with Array
private boolean Player = false;
private int PlayerMark = 1;

/*
 Player is the Current players turn. if false player 1 will play else player 2
 PlayerMark is to set number to the array "1" for player 1 and "2" for player 2
 */
Board() {

    // creates the panel
    setLayout(new GridLayout(3, 3));

    Buttons = new JButton[9];

    for (int i = 0; i < Buttons.length; i++) {
        Buttons[i] = new JButton("");
    }

    SetGame();

    for (int i = 0; i < Buttons.length; i++) {
        Buttons[i].addActionListener(this);
        add(Buttons[i]);
    }


}

public void SetGame() {

    GArray = new GameArray(this);

    DefaultText();
    DisableAll(true);

    Player = false; // default Value
    PlayerMark = 1; // default Value
}

public void Reset() {

    SetGame(); // To Reset the Game

}

public void actionPerformed(ActionEvent E) {

    JButton Pressed = (JButton) E.getSource();

    /*
     if any button is pressed the value is sent to GameArray class
     */
    if (Pressed == Buttons[0]) {
        GArray.ArrayInitialize(0, 0, PlayerMark);
        SetText(Pressed, Player);     // chaneg button text to "X" or "O" based on player turn
        PlayerMark = SwithTurn(Player); // Swithch Turns
        ButtonDisabler(Buttons[0]); // Disable pressed Button

    } else if (Pressed == Buttons[1]) {
        GArray.ArrayInitialize(0, 1, PlayerMark);
        SetText(Pressed, Player);
        SetText(Pressed, Player);
        PlayerMark = SwithTurn(Player);
        ButtonDisabler(Buttons[1]);
    } else if (Pressed == Buttons[2]) {
        GArray.ArrayInitialize(0, 2, PlayerMark);
        SetText(Pressed, Player);
        PlayerMark = SwithTurn(Player);
        ButtonDisabler(Buttons[2]);

    } else if (Pressed == Buttons[3]) {
        GArray.ArrayInitialize(1, 0, PlayerMark);
        SetText(Pressed, Player);
        PlayerMark = SwithTurn(Player);
        ButtonDisabler(Buttons[3]);

    } else if (Pressed == Buttons[4]) {
        GArray.ArrayInitialize(1, 1, PlayerMark);
        SetText(Pressed, Player);
        PlayerMark = SwithTurn(Player);
        ButtonDisabler(Buttons[4]);

    } else if (Pressed == Buttons[5]) {
        GArray.ArrayInitialize(1, 2, PlayerMark);
        SetText(Pressed, Player);
        PlayerMark = SwithTurn(Player);
        ButtonDisabler(Buttons[5]);
    } else if (Pressed == Buttons[6]) {
        GArray.ArrayInitialize(2, 0, PlayerMark);
        SetText(Pressed, Player);
        PlayerMark = SwithTurn(Player);
        ButtonDisabler(Buttons[6]);
    } else if (Pressed == Buttons[7]) {
        GArray.ArrayInitialize(2, 1, PlayerMark);
        SetText(Pressed, Player);
        PlayerMark = SwithTurn(Player);
        ButtonDisabler(Buttons[7]);
    } else if (Pressed == Buttons[8]) {
        GArray.ArrayInitialize(2, 2, PlayerMark);
        SetText(Pressed, Player);
        PlayerMark = SwithTurn(Player);
        ButtonDisabler(Buttons[8]);
    }

}

public int SwithTurn(boolean last) {

    // if the past player was false(player 1) then swith to true(player 2)

    if (last == true) {
        Player = false;
        return 1;

    } else if (last == false) {
        Player = true;
        return 2;
    } else {
        return 3;
    }

}

public void ButtonDisabler(JButton Btn) {

    Btn.setEnabled(false); // Disable Button

}

public void DisableAll(boolean Opp) {

            // Disable  All Buttons
    for (int i = 0; i < Buttons.length; i++) {
        Buttons[i].setEnabled(Opp);
    }

}

public void SetText(JButton Btn, boolean Play) {

    if (Play == true) {
        Btn.setText("O");
    } else if (Play == false) {
        Btn.setText("X");
    }

}

public void DefaultText() {

    for (int i = 0; i < Buttons.length; i++) {
        Buttons[i].setText("");
    }
}

}
\$\endgroup\$

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.