Results 1 to 5 of 5
  1. #1
    kiregad is offline Member
    Join Date
    Mar 2010
    Posts
    23
    Rep Power
    0

    Default I need feedback on my TicTacToe game

    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:

    Java Code:
    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]);
    			}
    		}
    	}
    }

  2. #2
    Fubarable's Avatar
    Fubarable is offline Moderator
    Join Date
    Jun 2008
    Posts
    19,315
    Blog Entries
    1
    Rep Power
    26

    Default

    Quote Originally Posted by kiregad View Post
    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:
    Java Code:
    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.

  3. #3
    kiregad is offline Member
    Join Date
    Mar 2010
    Posts
    23
    Rep Power
    0

    Default

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

  4. #4
    gcalvin is offline Senior Member
    Join Date
    Mar 2010
    Posts
    952
    Rep Power
    5

    Default

    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:
    Java Code:
        /**
         * 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-

  5. #5
    gcalvin is offline Senior Member
    Join Date
    Mar 2010
    Posts
    952
    Rep Power
    5

    Default

    Here's a version that's more in keeping with your original code:
    Java 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-
    Last edited by gcalvin; 03-21-2010 at 11:23 PM. Reason: Should have tested before posting

Similar Threads

  1. website section feedback
    By MuslimCoder in forum Reviews / Advertising
    Replies: 0
    Last Post: 02-25-2009, 08:35 PM
  2. Confluence or other wiki Feedback required
    By priyanka.dandekar in forum Web Frameworks
    Replies: 0
    Last Post: 02-24-2009, 09:32 AM
  3. testing midi pedal feedback
    By willemjav in forum New To Java
    Replies: 0
    Last Post: 10-04-2008, 12:55 AM
  4. TicTacToe Game
    By Ebtihal in forum New To Java
    Replies: 0
    Last Post: 01-09-2008, 12:01 PM

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •