I need feedback on my TicTacToe game

[kiregad]

import javax.swing.*; import - Anonymous - 98xr5fx6 - Pastebin.com

I started learning java at my uni this year, and I would preciate som good feedback. How is the oo, and the encaspulation etc?

Any feedback that can make me a better coder is gladly accepted :)

The whole code here:

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

public class TicTac extends JFrame {
	String currentPlayer = "X";
	boolean haveWon;
	static int moves = 0;

	int[][] win = new int[][] {
		{0,1,2}, {3,4,5}, {6,7,8}, // horizontal wins
		{0,3,6}, {1,4,7}, {2,5,8}, // vertical
		{0,4,8}, {2,4,6}           // diagonally
	};

	BoardPanel panel = new BoardPanel();
	JLabel status = new JLabel("Start playing. X is first.");

	public static void main(String[] args) { new TicTac(); }  // main mathod to fire tha game

	public TicTac() {
		setLayout(new BorderLayout());


		add(status, BorderLayout.SOUTH);
		add(panel, BorderLayout.NORTH);

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

	public void swapCurrentPlayer() {
		if(currentPlayer.equals("X"))
			currentPlayer = "O";
		else
			currentPlayer = "X";

		setStatus(currentPlayer+ " it\'s you turn.");
	}

	public String getCurrentPlayer() {
		return currentPlayer;
	}

	public void setStatus(String s)
	{
		status.setText(s);
	}

	// inner static class
	class BoardPanel extends JPanel {
		JLabel[] lbs = new JLabel[9];

		public BoardPanel() {
			// makes sure of nice a 3 * 3 table
			setLayout(new GridLayout(3, 3));

			// 9 labels to hold X or O


			for(int i=1;i < 10;i++) {
				lbs[i-1] = new JLabel(" ");
				lbs[i-1].setBorder(BorderFactory.createLineBorder(Color.black));

				// adds anonymous inner listenerclass for clicking the JLabels
				lbs[i-1].addMouseListener(new MouseAdapter() {
					public void mouseClicked(MouseEvent e) {
						Object o = e.getSource();
						for(int i=0;i < 9;i++) {
							if(o == lbs[i] && lbs[i].getText().equals(" ")) {
								lbs[i].setText(getCurrentPlayer());
								moves++;


							}
						}

						// Check for winner
						for(int i=0;i <= 7;i++) {
									if(lbs[win[i][0]].getText().equals(lbs[win[i][1]].getText()) &&
										lbs[win[i][1]].getText().equals(lbs[win[i][2]].getText()) &&
										lbs[win[i][0]].getText() != " ") { haveWon = true;}

						}

						// Got a winner?
						if(haveWon) {
							setStatus("And the winner is: "+getCurrentPlayer());
						}
						else
						{
							// check for tie
							if(moves == 9) { // no more room!
								setStatus("DRAW!");


							}
							else {	swapCurrentPlayer(); }
						}
					}
				});

				add(lbs[i-1]);
			}
		}
	}
}

[Fubarable]

I started learning java at my uni this year, and I would preciate som good feedback. How is the oo, and the encaspulation etc?

Any feedback that can make me a better coder is gladly accepted :)

The whole code here:

First, I'd like to repost your code with smaller indents as this makes it easier to read on the forum:

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

public class TicTac extends JFrame {
  String currentPlayer = "X";
  boolean haveWon;
  static int moves = 0;

  int[][] win = new int[][]{{0, 1, 2}, {3, 4, 5}, {6, 7, 8}, // horizontal wins
    {0, 3, 6}, {1, 4, 7}, {2, 5, 8}, // vertical
    {0, 4, 8}, {2, 4, 6} // diagonally
  };

  BoardPanel panel = new BoardPanel();
  JLabel status = new JLabel("Start playing. X is first.");

  public static void main(String[] args) {
    new TicTac();
  } // main mathod to fire tha game

  public TicTac() {
    setLayout(new BorderLayout());

    add(status, BorderLayout.SOUTH);
    add(panel, BorderLayout.NORTH);

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

  public void swapCurrentPlayer() {
    if (currentPlayer.equals("X"))
      currentPlayer = "O";
    else
      currentPlayer = "X";

    setStatus(currentPlayer + " it\'s you turn.");
  }

  public String getCurrentPlayer() {
    return currentPlayer;
  }

  public void setStatus(String s) {
    status.setText(s);
  }

  // inner static class
  class BoardPanel extends JPanel {
    JLabel[] lbs = new JLabel[9];

    public BoardPanel() {
      // makes sure of nice a 3 * 3 table
      setLayout(new GridLayout(3, 3));

      // 9 labels to hold X or O

      for (int i = 1; i < 10; i++) {
        lbs[i - 1] = new JLabel(" ");
        lbs[i - 1].setBorder(BorderFactory.createLineBorder(Color.black));

        // adds anonymous inner listenerclass for clicking the JLabels
        lbs[i - 1].addMouseListener(new MouseAdapter() {
          public void mouseClicked(MouseEvent e) {
            Object o = e.getSource();
            for (int i = 0; i < 9; i++) {
              if (o == lbs[i] && lbs[i].getText().equals(" ")) {
                lbs[i].setText(getCurrentPlayer());
                moves++;

              }
            }

            // Check for winner
            for (int i = 0; i <= 7; i++) {
              if (lbs[win[i][0]].getText().equals(lbs[win[i][1]].getText())
                  && lbs[win[i][1]].getText().equals(lbs[win[i][2]].getText())
                  && lbs[win[i][0]].getText() != " ") {
                haveWon = true;
              }

            }

            // Got a winner?
            if (haveWon) {
              setStatus("And the winner is: " + getCurrentPlayer());
            } else {
              // check for tie
              if (moves == 9) { // no more room!
                setStatus("DRAW!");

              } else {
                swapCurrentPlayer();
              }
            }
          }
        });

        add(lbs[i - 1]);
      }
    }
  }
}

Next for recs, well
1) You've got everything tied up in one class -- GUI / logic / controls. You may wish to separate these components out into separate classes? Why? if you do this, it would make this code (and future code) easier to debug and test, easier to upgrade the GUI without fear of messing up the logic, easier to port to other GUIs (i.e., smart phones), easier to add new functionality such as AI.

2) The class subclasses JFrame but really doesn't need to or want to do this as you never override any JFrame methods. Better to have it extend no class but to create a JPanel that can be displayed in a JFrame if desired or a JApplet if desired.

By the way, I do like your use of a solution matrix.

Best of luck and welcome to the forum.

[kiregad]

Thanks for quick feedback. I will try to improve the code.

[gcalvin]

I agree with everything Fubarable said. Also, I think you would benefit greatly from breaking your code up into methods. Right now you have almost all of your game logic crammed into your mouseClicked() method. It's hard to read and confusing.

Consider how you might break out this code, for example:

    /**
     * Checks solution matrix wins to see if we have a winner
     *
     * @return String "X" or "O" or "N" for no winner
     */
    private String checkForWinner() {
        String mark = "N";
        for (int[] w : win) { // iterate through the arrays in the solution matrix
            for (int i = 0; i < 3; i++) {
                String s = lbs[w[i]].getText();
                if (s.equals("")) break; // no winner for this pattern
                if (i == 0) mark = s; 
                if (!mark.equals(s)) break; // no winner
                if (i == 2) {
                    // we've checked the last of the three squares, and it
                    // matches the first two. we have a winner -- return X or O
                    return mark;
                }
            }
        }
        return "N"; // no winner
    }

OK, I'm not sure I like it myself, but I hope it at least gives you some ideas. The point is to encapsulate the logic in a method with a clear name that does what it promises to do. Try to write short methods, each of which does one thing.

-Gary-

[gcalvin]

Here's a version that's more in keeping with your original code:

    /**
     * Checks solution matrix wins to see if we have a winner
     *
     * @return boolean true if there is a winner, false if not
     */
    private boolean checkForWinner() {
        for (int[] w : win) { // iterate through the arrays in the solution matrix
            if (! lbs[w[0]].getText().equals(" ") &&
                lbs[w[0]].getText().equals(lbs[w[1]].getText()) &&
                lbs[w[1]].getText().equals(lbs[w[2]].getText())) {
                return true;
            }
        }
        return false;
    }

I guess I like that better myself.

-Gary-