5
\$\begingroup\$

I made two player objects (p1 and p2):

public class tttmodimproved{
    static int turn=0;
    public static void main(String[] args){
        System.out.println("Let's play TicTacToe");
        char isWin=' ';
        Board b= new Board();
        Player p1=new Player('X');
        Player p2= new Player('O');
        Player p;
        for(b.tc=0;b.tc<9;b.tc++){
            p=cp(p1,p2);
            b.updateBoard(p,p.getMove(b));
            isWin=b.checkWin(p);
            b.showBoard();
            if(isWin!=' ')
                break;
        }
         if(isWin!=' ')
            System.out.println("congrats! " + isWin +" won the game.");
        else
            System.out.println("tie");

    }
    static Player cp(Player p1,Player p2){

        if(turn==0)
        {
            turn=1;
            return p1;
        }
        else if(turn==1)
        {
            turn=0;
            return p2;
        }
        else
            return p1;
    }
    }
    class Board{
    char[] s={' ',' ',' ',' ',' ',' ',' ',' ',' '};
    int tc=0;


    char checkWin(Player p){
        char m=p.mark;
        if(m==((s[1]&s[4]&s[7])
            |(s[0]&s[3]&s[6])
            |(s[2]&s[5]&s[8])
            |(s[6]&s[7]&s[8])
            |(s[3]&s[4]&s[5])
            |(s[0]&s[1]&s[2])
            |(s[2]&s[4]&s[6])
            |(s[0]&s[4]&s[8]))){
            return m;}
        return (' ');
    }
    void showBoard(){
        System.out.println("");
        System.out.println(" "+s[6]+" | "+s[7]+" | "+s[8]+" ");
        System.out.println("---|---|---");
        System.out.println(" "+s[3]+" | "+s[4]+" | "+s[5]+" ");
        System.out.println("---|---|---");
        System.out.println(" "+s[0]+" | "+s[1]+" | "+s[2]+" ");
        System.out.println("");
    }
    void updateBoard(Player p,int move){
        s[move]=p.mark;
    }

    }
    class Player{
    char mark=' ',markOpo=' ';

    Player(char mark){
        this.mark=mark;
        System.out.println("player created with mark: " +this.mark);
        if(this.mark=='X'){
            markOpo='O';
        }
        else
            markOpo='X';
    }
    int getMove(Board b){
        int move=9;
        if(b.tc>2){
            move=check(mark,b);
            if(move==9){
                move=check(markOpo,b);
            }
        }
        while((move>8) || (b.s[move]!=' ')){
            move=(int)(Math.random()*9);
        }
        return move;
    }
    int check(char m,Board b){
        int mo;
        mo=checkArray(6,7,8,m,b);

        if(mo!=9)
            return mo;
        mo=checkArray(3,4,5,m,b);

        if(mo!=9)
            return mo;
        mo=checkArray(0,1,2,m,b);

        if(mo!=9)
            return mo;
        mo=checkArray(0,3,6,m,b);

        if(mo!=9)
            return mo;
        mo=checkArray(1,4,7,m,b);

        if(mo!=9)
            return mo;
        mo=checkArray(2,5,8,m,b);

        if(mo!=9)
            return mo;
        mo=checkArray(0,4,8,m,b);

        if(mo!=9)
            return mo;
        mo=checkArray(2,4,6,m,b);

        if(mo!=9)
            return mo;
        else
            return mo;

        }
    int checkArray(int a,int d,int c,char m,Board b){
        if((m==(b.s[a]&b.s[d]))&((b.s[c])==' ')){
            return c;
        }
        else if(((b.s[a]&b.s[c])==m)&&(b.s[d]==' '))
            return d;
        else if(((b.s[d]&b.s[c])==m)&&(b.s[a]==' '))
            return a;
        else
            return 9;
        }

    }
\$\endgroup\$
3
  • 1
    \$\begingroup\$ To help reviewers give you better answers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question. \$\endgroup\$ Nov 25, 2015 at 17:07
  • 1
    \$\begingroup\$ If both players are playing optimally, then Tic-Tac-Toe should end in a draw every time. I'd say that you have random players rather than AI players. \$\endgroup\$ Nov 25, 2015 at 17:35
  • \$\begingroup\$ Yes the moves that doesn't have a winning or loosing chance is given randomly, after all this is just a SIMPLE AI operated game, nothing complex. \$\endgroup\$ Nov 26, 2015 at 0:11

2 Answers 2

6
\$\begingroup\$

Bugs

if(m==((s[1]&s[4]&s[7])
        |(s[0]&s[3]&s[6])
        |(s[2]&s[5]&s[8])
        |(s[6]&s[7]&s[8])
        |(s[3]&s[4]&s[5])
        |(s[0]&s[1]&s[2])
        |(s[2]&s[4]&s[6])
        |(s[0]&s[4]&s[8]))){

Does this even work? It seems to me that you are trying to test m to each of the above values, but what you are really doing is a binary math operation of & and |. Let me explain...

& "and"s each of the bits in the number, e.g.:

13&19

13:   0 1 1 0 1
17: & 1 0 0 0 1
      0 0 0 0 1

See what happened? 1 and 1 becomes 1, and every other combination becomes 0.

| "or"s the bits:

13:   0 1 1 0 1
17: | 1 0 0 0 1
      1 1 1 0 1

Similar to &, but 0 and 0 result in 0, and every other combination 1.

What is happening is that the char ASCII values are &'d and |'d. Is this really what you want? In this case, you have to do it the tedious way: test each of them to m:

    if (((m == board[1] && m == board[4] && m == board[7])
            || (m == board[0] && m == board[3] && m == board[6])
            || (m == board[2] && m == board[5] && m == board[8])
            || (m == board[6] && m == board[7] && m == board[8])
            || (m == board[3] && m == board[4] && m == board[5])
            || (m == board[0] && m == board[1] && m == board[2])
            || (m == board[2] && m == board[4] && m == board[6])
            || (m == board[0] && m == board[4] && m == board[8]))) {
        return m;
    }

Same here:

if((m==(b.s[a]&b.s[d]))&((b.s[c])==' ')){
    return c;
}
else if(((b.s[a]&b.s[c])==m)&&(b.s[d]==' '))
    return d;
else if(((b.s[d]&b.s[c])==m)&&(b.s[a]==' '))
    return a;
else
    return 9;
}

Becomes:

int checkArray(int a, int d, int c, Mark m, Board b) {
    if ((m == b.getBoard()[a] && m == b.getBoard()[d]) && ((b.getBoard()[c]) == Mark.NONE)) {
        return c;
    } else if ((m == b.getBoard()[a] && m == b.getBoard()[c]) && (b.getBoard()[d] == Mark.NONE)) {
        return d;
    } else if ((m == b.getBoard()[d] && m == b.getBoard()[c]) && (b.getBoard()[a] == Mark.NONE)) {
        return a;
    }
    return 9;
}

Formatting

I can't even read your code. Format it better; in an IDE, you can find a formatting option. In eclipse, it's Source->Format.

If you don't use an IDE, I suggest you install one. Eclipse is available here. Choose the installer, or the "Eclipse IDE for Java Developers" option.

Naming

public class tttmodimproved{

tttmodimproved is not only a bad name, it is also hard to read and understand. Cramming too much information in names is bad. Sure, names like class are bad, but so is thisclassdoesfooandbarwhilealsodoingotherstuff. That's what class-level javadoc comments are for.

As for readability, java class names are in PascalCase, and method and variable names in camelCase.

I suggest TicTacToe, and you can add a short comment about the class at the beginning, like so:

/**
 * ...
 */
public class TicTacToe {
    // ...

Other naming fixes:

  • cp -> changePlayer
  • p1 -> player1
  • p2 -> player2
  • b -> board
  • p -> currentPlayer
  • s -> board
  • tc: What does this even do?
  • ...

I give up. Let's see what we have so far:

public class TicTacToe {

    static int turn = 0;

    public static void main(String[] args) {
        System.out.println("Let's play TicTacToe");
        char isWin = ' ';
        Board board = new Board();
        Player player1 = new Player('X');
        Player player2 = new Player('O');
        Player currentPlayer;
        for (board.tc = 0; board.tc < 9; board.tc++) {
            currentPlayer = changePlayer(player1, player2);
            board.updateBoard(currentPlayer, currentPlayer.getMove(board));
            isWin = board.checkWin(currentPlayer);
            board.showBoard();
            if (isWin != ' ')
                break;
        }
        if (isWin != ' ')
            System.out.println("congrats! " + isWin + " won the game.");
        else
            System.out.println("tie");

    }

    static Player changePlayer(Player p1, Player p2) {
        if (turn == 0) {
            turn = 1;
            return p1;
        } else if (turn == 1) {
            turn = 0;
            return p2;
        } else
            return p1;
    }

}

class Board {

    char[] board = { ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ' };
    int tc = 0;

    char checkWin(Player p) {
        char m = p.mark;
        if (m == ((board[1] & board[4] & board[7]) | (board[0] & board[3] & board[6]) | (board[2] & board[5] & board[8])
                | (board[6] & board[7] & board[8]) | (board[3] & board[4] & board[5]) | (board[0] & board[1] & board[2])
                | (board[2] & board[4] & board[6]) | (board[0] & board[4] & board[8]))) {
            return m;
        }
        return (' ');
    }

    void showBoard() {
        System.out.println("");
        System.out.println(" " + board[6] + " | " + board[7] + " | " + board[8] + " ");
        System.out.println("---|---|---");
        System.out.println(" " + board[3] + " | " + board[4] + " | " + board[5] + " ");
        System.out.println("---|---|---");
        System.out.println(" " + board[0] + " | " + board[1] + " | " + board[2] + " ");
        System.out.println("");
    }

    void updateBoard(Player player, int move) {
        board[move] = player.mark;
    }

}

class Player {
    char mark = ' ', markOpo = ' ';

    Player(char mark) {
        this.mark = mark;
        System.out.println("player created with mark: " + this.mark);
        if (this.mark == 'X') {
            markOpo = 'O';
        } else
            markOpo = 'X';
    }

    int getMove(Board b) {
        int move = 9;
        if (b.tc > 2) {
            move = check(mark, b);
            if (move == 9) {
                move = check(markOpo, b);
            }
        }
        while ((move > 8) || (b.board[move] != ' ')) {
            move = (int) (Math.random() * 9);
        }
        return move;
    }

    int check(char m, Board b) {
        int mo;
        mo = checkArray(6, 7, 8, m, b);

        if (mo != 9)
            return mo;
        mo = checkArray(3, 4, 5, m, b);

        if (mo != 9)
            return mo;
        mo = checkArray(0, 1, 2, m, b);

        if (mo != 9)
            return mo;
        mo = checkArray(0, 3, 6, m, b);

        if (mo != 9)
            return mo;
        mo = checkArray(1, 4, 7, m, b);

        if (mo != 9)
            return mo;
        mo = checkArray(2, 5, 8, m, b);

        if (mo != 9)
            return mo;
        mo = checkArray(0, 4, 8, m, b);

        if (mo != 9)
            return mo;
        mo = checkArray(2, 4, 6, m, b);

        if (mo != 9)
            return mo;
        else
            return mo;

    }

    int checkArray(int a, int d, int c, char m, Board b) {
        if ((m == (b.board[a] & b.board[d])) & ((b.board[c]) == ' ')) {
            return c;
        } else if (((b.board[a] & b.board[c]) == m) && (b.board[d] == ' '))
            return d;
        else if (((b.board[d] & b.board[c]) == m) && (b.board[a] == ' '))
            return a;
        else
            return 9;
    }

}

Now let's do some actual reviewing...

OOP

Java is an Object Oriented Programming (OOP) language, which means it's based on objects. You did a good job trying, but if you leave a default constructor and let the calling code do the work, it's not OOP at all. Let's do some serious redesigning:

public class TicTacToe {

    public static void main(String[] args) {
        System.out.println("Let's play TicTacToe");
        Mark win = Mark.NONE;
        Board board = new Board();
        Player player1 = new Player(Mark.X);
        Player player2 = new Player(Mark.O);
        Player currentPlayer = null;
        for (int tc = 0; tc < 9; tc++) {
            currentPlayer = switchPlayer(currentPlayer, player1, player2);
            board.updateBoard(currentPlayer, currentPlayer.getMove(board));
            win = board.checkWin(currentPlayer);
            board.showBoard();
            if (win != Mark.NONE) {
                break;
            }
        }
        if (win != Mark.NONE)
            System.out.println("congrats! " + win + " won the game.");
        else
            System.out.println("tie");

    }

    private static Player switchPlayer(Player currentPlayer, Player player1, Player player2) {
        if (currentPlayer == player1) {
            return player2;
        }
        return player1;
    }

}

class Board {

    private Mark[] board = { Mark.NONE, Mark.NONE, Mark.NONE, Mark.NONE, Mark.NONE, Mark.NONE, Mark.NONE, Mark.NONE, Mark.NONE };
    private int tc = 0;

    public Board() {

    }

    public Mark[] getBoard() {
        return board;
    }

    public int getTC() {
        return tc;
    }

    public void setTC(int tc) {
        this.tc = tc;
    }

    public Mark checkWin(Player p) {
        Mark m = p.getMark();
        if (((m == board[1] && m == board[4] && m == board[7]) || (m == board[0] && m == board[3] && m == board[6])
                || (m == board[2] && m == board[5] && m == board[8])
                || (m == board[6] && m == board[7] && m == board[8])
                || (m == board[3] && m == board[4] && m == board[5])
                || (m == board[0] && m == board[1] && m == board[2])
                || (m == board[2] && m == board[4] && m == board[6])
                || (m == board[0] && m == board[4] && m == board[8]))) {
            return m;
        }
        return Mark.NONE;
    }

    public void showBoard() {
        System.out.println("");
        System.out.println(" " + board[6] + " | " + board[7] + " | " + board[8] + " ");
        System.out.println("---|---|---");
        System.out.println(" " + board[3] + " | " + board[4] + " | " + board[5] + " ");
        System.out.println("---|---|---");
        System.out.println(" " + board[0] + " | " + board[1] + " | " + board[2] + " ");
        System.out.println("");
    }

    public void updateBoard(Player player, int move) {
        board[move] = player.getMark();
    }

}

class Player {

    private Mark mark = Mark.NONE;
    private Mark markOpo = Mark.NONE;

    public Player(Mark mark) {
        this.mark = mark;
        System.out.println("player created with mark: " + this.mark);
        if (this.mark == Mark.X) {
            markOpo = Mark.O;
        } else {
            markOpo = Mark.X;
        }
    }

    public Mark getMark() {
        return mark;
    }

    public int getMove(Board b) {
        int move = 9;
        if (b.getTC() > 2) {
            move = check(mark, b);
            if (move == 9) {
                move = check(markOpo, b);
            }
        }
        while (move > 8 || b.getBoard()[move] != Mark.NONE) {
            move = (int) (Math.random() * 9);
        }
        return move;
    }

    int check(Mark mark, Board board) {
        int mo;
        mo = checkArray(6, 7, 8, mark, board);
        if (mo != 9) 
            return mo;
        mo = checkArray(3, 4, 5, mark, board);

        if (mo != 9) 
            return mo;
        mo = checkArray(0, 1, 2, mark, board);

        if (mo != 9)
            return mo;
        mo = checkArray(0, 3, 6, mark, board);

        if (mo != 9)
            return mo;
        mo = checkArray(1, 4, 7, mark, board);

        if (mo != 9)
            return mo;
        mo = checkArray(2, 5, 8, mark, board);

        if (mo != 9)
            return mo;
        mo = checkArray(0, 4, 8, mark, board);

        if (mo != 9)
            return mo;
        mo = checkArray(2, 4, 6, mark, board);

        if (mo != 9)
            return mo;
        else
            return mo;

    }

    private int checkArray(int a, int d, int c, Mark m, Board b) {
        if ((m == b.getBoard()[a] && m == b.getBoard()[d]) && ((b.getBoard()[c]) == Mark.NONE)) {
            return c;
        } else if ((m == b.getBoard()[a] && m == b.getBoard()[c]) && (b.getBoard()[d] == Mark.NONE)) {
            return d;
        } else if ((m == b.getBoard()[d] && m == b.getBoard()[c]) && (b.getBoard()[a] == Mark.NONE)) {
            return a;
        }
        return 9;
    }

}

enum Mark {
    NONE, X, O;

    @Override
    public String toString() {
        switch (this) {
        case NONE:
            return " ";
        case O:
            return "O";
        case X:
            return "X";
        default:
            return "";
        }
    }
}

A little explanation in the changes I made:

  • char is now Mark. It's more OOP that way, as char don't exactly represent a mark that a player makes.
  • default level methods are now either public or private. OOP people don't like default privacy methods.
  • turn variable in the Main class is now gone.

Now, moving to other changes:

The check method can be improved.

    if (mo != 9)
        return mo;
    else
        return mo;

This statement is redundant. It says, if mo is not 9, then return the value of mo. Otherwise, return the value of mo.

That can easily be:

    return mo;

    mo = checkArray(6, 7, 8, mark, board);
    if (mo != 9) 
        return mo;
    mo = checkArray(3, 4, 5, mark, board);

    if (mo != 9) 
        return mo;
    mo = checkArray(0, 1, 2, mark, board);

    if (mo != 9)
        return mo;
    mo = checkArray(0, 3, 6, mark, board);

    if (mo != 9)
        return mo;
    mo = checkArray(1, 4, 7, mark, board);

    if (mo != 9)
        return mo;
    mo = checkArray(2, 5, 8, mark, board);

    if (mo != 9)
        return mo;
    mo = checkArray(0, 4, 8, mark, board);

    if (mo != 9)
        return mo;
    mo = checkArray(2, 4, 6, mark, board);

A lot of repetition here. Use loops:

int check(Mark mark, Board board) {
    int mo;
    // rows
    for (int i = 0; i < 9; i += 3) {
        mo = checkArray(i, i + 1, i + 2, mark, board);
        if (mo != 9) {
            return mo;
        }
    }
    // columns
    for (int i = 0; i < 3; i++) {
        mo = checkArray(i, i + 3, i + 6, mark, board);
        if (mo != 9) {
            return mo;
        }
    }
    // diagonals
    mo = checkArray(0, 4, 8, mark, board);
    if (mo != 9) {
        return mo;
    }

    mo = checkArray(2, 4, 6, mark, board);
    return mo;

}

Final Code:

public class TicTacToe {

    public static void main(String[] args) {
        System.out.println("Let's play TicTacToe");
        Mark win = Mark.NONE;
        Board board = new Board();
        Player player1 = new Player(Mark.X);
        Player player2 = new Player(Mark.O);
        Player currentPlayer = null;
        for (int tc = 0; tc < 9; tc++) {
            currentPlayer = switchPlayer(currentPlayer, player1, player2);
            board.updateBoard(currentPlayer, currentPlayer.getMove(board));
            win = board.checkWin(currentPlayer);
            board.showBoard();
            if (win != Mark.NONE) {
                break;
            }
        }
        if (win != Mark.NONE)
            System.out.println("congrats! " + win + " won the game.");
        else
            System.out.println("tie");

    }

    private static Player switchPlayer(Player currentPlayer, Player player1, Player player2) {
        if (currentPlayer == player1) {
            return player2;
        }
        return player1;
    }

}

class Board {

    private Mark[] board = { Mark.NONE, Mark.NONE, Mark.NONE, Mark.NONE, Mark.NONE, Mark.NONE, Mark.NONE, Mark.NONE, Mark.NONE };
    private int tc = 0;

    public Board() {

    }

    public Mark[] getBoard() {
        return board;
    }

    public int getTC() {
        return tc;
    }

    public void setTC(int tc) {
        this.tc = tc;
    }

    public Mark checkWin(Player p) {
        Mark m = p.getMark();
        if (((m == board[1] && m == board[4] && m == board[7]) || (m == board[0] && m == board[3] && m == board[6])
                || (m == board[2] && m == board[5] && m == board[8])
                || (m == board[6] && m == board[7] && m == board[8])
                || (m == board[3] && m == board[4] && m == board[5])
                || (m == board[0] && m == board[1] && m == board[2])
                || (m == board[2] && m == board[4] && m == board[6])
                || (m == board[0] && m == board[4] && m == board[8]))) {
            return m;
        }
        return Mark.NONE;
    }

    public void showBoard() {
        System.out.println("");
        System.out.println(" " + board[6] + " | " + board[7] + " | " + board[8] + " ");
        System.out.println("---|---|---");
        System.out.println(" " + board[3] + " | " + board[4] + " | " + board[5] + " ");
        System.out.println("---|---|---");
        System.out.println(" " + board[0] + " | " + board[1] + " | " + board[2] + " ");
        System.out.println("");
    }

    public void updateBoard(Player player, int move) {
        board[move] = player.getMark();
    }

}

class Player {

    private Mark mark = Mark.NONE;
    private Mark markOpo = Mark.NONE;

    public Player(Mark mark) {
        this.mark = mark;
        System.out.println("player created with mark: " + this.mark);
        if (this.mark == Mark.X) {
            markOpo = Mark.O;
        } else {
            markOpo = Mark.X;
        }
    }

    public Mark getMark() {
        return mark;
    }

    public int getMove(Board board) {
        int move = 9;
        if (board.getTC() > 2) {
            move = check(mark, board);
            if (move == 9) {
                move = check(markOpo, board);
            }
        }
        while (move > 8 || board.getBoard()[move] != Mark.NONE) {
            move = (int) (Math.random() * 9);
        }
        return move;
    }

    int check(Mark mark, Board board) {
        int mo;
        // rows
        for (int i = 0; i < 9; i += 3) {
            mo = checkArray(i, i + 1, i + 2, mark, board);
            if (mo != 9) {
                return mo;
            }
        }
        // columns
        for (int i = 0; i < 3; i++) {
            mo = checkArray(i, i + 3, i + 6, mark, board);
            if (mo != 9) {
                return mo;
            }
        }
        // diagonals
        mo = checkArray(0, 4, 8, mark, board);
        if (mo != 9) {
            return mo;
        }

        mo = checkArray(2, 4, 6, mark, board);
        return mo;

    }

    private int checkArray(int a, int d, int c, Mark m, Board b) {
        if ((m == b.getBoard()[a] && m == b.getBoard()[d]) && ((b.getBoard()[c]) == Mark.NONE)) {
            return c;
        } else if ((m == b.getBoard()[a] && m == b.getBoard()[c]) && (b.getBoard()[d] == Mark.NONE)) {
            return d;
        } else if ((m == b.getBoard()[d] && m == b.getBoard()[c]) && (b.getBoard()[a] == Mark.NONE)) {
            return a;
        }
        return 9;
    }

}

enum Mark {
    NONE, X, O;

    @Override
    public String toString() {
        switch (this) {
        case NONE:
            return " ";
        case O:
            return "O";
        case X:
            return "X";
        default:
            return "";
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

Without even looking at how the code works, I see some serious readability issues here, among which:

To {} or not to {}

Be consistent on your coding choices. For example, sometimes you choose to put {} brackets even for single-instruction-body ifs and sometimes you don't. Make a choice on which style you're adopting (personally I'd suggest to put {} even for single-instruction-body ifs) and stick to it.

Naming

Avoid single letter variables/fields. Give them a meaningful name.

Methods should have meaningful names also. What does checkArray() do?

Indentation

The same as the first point. Be consistent in it.

\$\endgroup\$
3
  • \$\begingroup\$ I'm always very bad at naming things, would you please suggests me some tips &/or books that can help me with that. \$\endgroup\$ Nov 26, 2015 at 0:07
  • 2
    \$\begingroup\$ @Lnk try to be more descriptive. If p is really a player, name it player then. And remember to follow Java's camelCase convention if you do not have an existing convention to enhance readability for other Java developers... \$\endgroup\$
    – h.j.k.
    Nov 26, 2015 at 1:14
  • \$\begingroup\$ @Lnk, h.j.k and TheCoffeeCup have done an excellent job in explaining how naming should be. Other than what they've already said, I'd only suggest to take a look at the official code conventions for java. \$\endgroup\$ Nov 26, 2015 at 6:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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