5
\$\begingroup\$

I'm just starting to learn Java and I have written this simple Tic Tac Toe game. Please review it and give me some feedback, advice, guidance etc.

Main method:

    import java.util.Scanner;

    public class Main {

public static void main(String[] args) {
        char player='o';
        int row,column;
        Scanner k =  new Scanner (System.in);
        Game g = new Game ();

        g.initializeGame();
        System.out.println("Game ready !\n");

while(true)
{   
player=g.changePlayer(player);
System.out.print("\n"+player +" ,choose your location (row, column):");
row=k.nextInt();
column=k.nextInt();

    while (g.checkIfLegal(row,column))
    {
    System.out.println("That slot is already taken or out of bounds, please try again (row, column).");
    g.displayBoard();
    row=k.nextInt();
    column=k.nextInt();
    }
g.changeBoard(player,row,column );
g.displayBoard();

if(g.checkIfWinner())
    {
    System.out.println("\nThe winner is "+ player+" !");
    break;

    }
if(g.checkIfTie())
    {
        System.out.println("\nThe game is a tie !");
        break;
    }
}


    }

    }

Game class:

    public class Game {

char [][] table = new char[3][3];



public void initializeGame() 
{
    for (int i = 0; i < 3; i++)
        for (int p=0; p < 3; p++)
            table [i][p]= ' ';
}


public boolean checkIfLegal(int row, int column) 
{
     if( (row>2 || column>2) || (row<0 || column <0) )
         return true;

else if(table[row][column]=='x' || table[row][column]=='o')
        return true;

        return false;
}


public void changeBoard(char player, int row, int column)
{
    table[row][column]=player;

}


public void displayBoard() 
{

    System.out.println("  0  " + table[0][0] + "|" + table[0][1] + "|" + table[0][2]);
    System.out.println("    --+-+--");
    System.out.println("  1  " + table[1][0] + "|" + table[1][1] + "|" + table[1][2]);
    System.out.println("    --+-+--");
    System.out.println("  2  " + table[2][0] + "|" + table[2][1] + "|" + table[2][2]);
    System.out.println("     0 1 2 ");
}


public char changePlayer(char player) {
    char newTurn='e';
    if (player == 'o')
        newTurn='x';
    if (player == 'x')
        newTurn='o';
    return newTurn;
}


public boolean checkIfWinner() {
         if( table [0][0]==table[1][0] && table[1][0] == table[2][0] && (table [0][0]=='x' || table [0][0]=='o'))
                return true;
    else if( table [0][1]==table[1][1] && table[1][1] == table[2][1] && (table [0][1]=='x' || table [0][1]=='o'))
                return true;
    else if( table [0][2]==table[1][2] && table[1][2] == table[2][2] && (table [0][2]=='x' || table [0][2]=='o'))
                return true;
    else if( table [0][0]==table[0][1] && table[0][1] == table[0][2] && (table [0][0]=='x' || table [0][0]=='o'))
                return true;
    else if( table [1][0]==table[1][1] && table[1][1] == table[1][2] && (table [1][0]=='x' || table [1][0]=='o'))
                return true;
    else if( table [2][0]==table[2][1] && table[2][1] == table[2][2] && (table [2][0]=='x' || table [2][0]=='o'))
                return true;
    else if( table [0][0]==table[1][1] && table[1][1] == table[2][2] && (table [0][0]=='x' || table [0][0]=='o'))
                return true;
    else if( table [2][0]==table[1][1] && table[1][1] == table[0][2] && (table [2][0]=='x' || table [2][0]=='o'))
                return true;
    else
                return false;
}


public boolean checkIfTie() {
    for (int i = 0; i < 3; i++)
        for (int p=0; p < 3; p++)
            if(table [i][p]==' ')
                return false;

    return true;
}



    }
\$\endgroup\$
  • \$\begingroup\$ Welcome to CodeReview. Basic is a family of languages, not meaning that something is "basic." I actually just finished a tic-tac-toe project myself. I hope you get some fine answers. P.S. After this, if you're interested, you can take it to the next level \$\endgroup\$ – Legato Jul 5 '15 at 15:16
7
\$\begingroup\$

Avoid magic symbols

The character literals 'x', 'o', ' ' appear many times throughout the code. It would be better to put these in constants, for example:

static final char EMPTY = ' ';
static final char PLAYER_X = 'x';
static final char PLAYER_O = 'o';

Simplify changePlayer

In changePlayer, the newTurn variable is pointless. The player is either 'o' or 'x', so you can simplify to:

public char changePlayer(char player) {
    return player == 'o' ? 'x' : 'o';
}

Simplify checkIfLegal

The parentheses are unnecessary in the boundary check condition.

And instead of checking if a cell is 'x' or 'o', it would be simpler to check if it's ' '.

Like this:

public boolean checkIfLegal(int row, int column) {
    if (row > 2 || column > 2 || row < 0 || column < 0) {
        return true;
    } else if (table[row][column] != ' ') {
        return true;
    }

    return false;
}

Not intuitive checkIfLegal

The return value of checkIfLegal is kind of counter-intuitive. Judging by the name, my first thought is to return true if legal. But that's not the case. A more natural name would be isIllegal.

Formatting

Your code, nicely formatted, would look below. I simply used my IDE to auto-reformat. You can do this too, next time.

public class Game {

    char[][] table = new char[3][3];

    public void initializeGame() {
        for (int i = 0; i < 3; i++) {
            for (int p = 0; p < 3; p++) {
                table[i][p] = ' ';
            }
        }
    }


    public boolean checkIfLegal(int row, int column) {
        if ((row > 2 || column > 2) || (row < 0 || column < 0)) {
            return true;
        } else if (table[row][column] == 'x' || table[row][column] == 'o') {
            return true;
        }

        return false;
    }


    public void changeBoard(char player, int row, int column) {
        table[row][column] = player;

    }


    public void displayBoard() {

        System.out.println("  0  " + table[0][0] + "|" + table[0][1] + "|" + table[0][2]);
        System.out.println("    --+-+--");
        System.out.println("  1  " + table[1][0] + "|" + table[1][1] + "|" + table[1][2]);
        System.out.println("    --+-+--");
        System.out.println("  2  " + table[2][0] + "|" + table[2][1] + "|" + table[2][2]);
        System.out.println("     0 1 2 ");
    }


    public char changePlayer(char player) {
        char newTurn = 'e';
        if (player == 'o') {
            newTurn = 'x';
        }
        if (player == 'x') {
            newTurn = 'o';
        }
        return newTurn;
    }


    public boolean checkIfWinner() {
        if (table[0][0] == table[1][0] && table[1][0] == table[2][0] && (table[0][0] == 'x' || table[0][0] == 'o')) {
            return true;
        } else if (table[0][1] == table[1][1] && table[1][1] == table[2][1] && (table[0][1] == 'x' || table[0][1] == 'o')) {
            return true;
        } else if (table[0][2] == table[1][2] && table[1][2] == table[2][2] && (table[0][2] == 'x' || table[0][2] == 'o')) {
            return true;
        } else if (table[0][0] == table[0][1] && table[0][1] == table[0][2] && (table[0][0] == 'x' || table[0][0] == 'o')) {
            return true;
        } else if (table[1][0] == table[1][1] && table[1][1] == table[1][2] && (table[1][0] == 'x' || table[1][0] == 'o')) {
            return true;
        } else if (table[2][0] == table[2][1] && table[2][1] == table[2][2] && (table[2][0] == 'x' || table[2][0] == 'o')) {
            return true;
        } else if (table[0][0] == table[1][1] && table[1][1] == table[2][2] && (table[0][0] == 'x' || table[0][0] == 'o')) {
            return true;
        } else if (table[2][0] == table[1][1] && table[1][1] == table[0][2] && (table[2][0] == 'x' || table[2][0] == 'o')) {
            return true;
        } else {
            return false;
        }
    }


    public boolean checkIfTie() {
        for (int i = 0; i < 3; i++) {
            for (int p = 0; p < 3; p++) {
                if (table[i][p] == ' ') {
                    return false;
                }
            }
        }

        return true;
    }


}


public class Main {

    public static void main(String[] args) {
        char player = 'o';
        int row, column;
        Scanner k = new Scanner(System.in);
        Game g = new Game();

        g.initializeGame();
        System.out.println("Game ready !\n");

        while (true) {
            player = g.changePlayer(player);
            System.out.print("\n" + player + " ,choose your location (row, column):");
            row = k.nextInt();
            column = k.nextInt();

            while (g.checkIfLegal(row, column)) {
                System.out.println("That slot is already taken or out of bounds, please try again (row, column).");
                g.displayBoard();
                row = k.nextInt();
                column = k.nextInt();
            }
            g.changeBoard(player, row, column);
            g.displayBoard();

            if (g.checkIfWinner()) {
                System.out.println("\nThe winner is " + player + " !");
                break;

            }
            if (g.checkIfTie()) {
                System.out.println("\nThe game is a tie !");
                break;
            }
        }


    }

}
\$\endgroup\$
  • \$\begingroup\$ do you think checkIfWinner() function should have that many if else ? or should for loop better option? \$\endgroup\$ – pokche Aug 22 '16 at 21:52
5
\$\begingroup\$

Before I could even get to your logic a lot of the things in your code stood out as necessary fixes.

On Convetion and Readability

In Java, it's conventional to have the curly braces begin on the same line as the method name with the inner code indented one level. This makes scope clear and will be godsend when you're working on multi-branch projects.

Like so:

public class Example {
    public static void main(String[] args) {
        boolean indented = false;

        if (!indented) {
            System.out.println("Use tabs or two spaces to indent your code." +
                "\nIt really makes it easy to track where everything is."
            );
        }
    }
}

It's also helpful with your multiple conditional branch, being on the same line is especially important to you since in a few cases you're omitting the braces, which I don't recommend - if/when you work on a project with multiple people, or just maintain your own older code it can be deceptively simple to cause unintended effects that proper line breaks, tabs, and braces would prevent.

Either way the most important part is whatever you do, be consistent.

Additionally, use more meaningful names for your variables. You have many that are simply one letter, it may not be as problematic now since it's smaller code, but If this were to get lengthy one might wonder just what g is, calling it g instead of the more descriptive game saves 3 keystrokes at the cost of confusion and possible blunders. This is true for most of your variables e.g. call your Scanner input, or keyboard instead of just k.

At this point You may be thinking to yourself that the compiler doesn't care about white-space, indentation, or any of this but as a beginner I feel it's important to tell you: It is a misrepresentation to think that code is simply a means of communicating with a machine, although that is true it is predominantly the means in which we communicate with other programmers and with that in mind I advocate you write your code for other people.

Onto logic

Your "game ready" print message should be a part of your game class. It makes a lot more sense if it was in its initializeGame method. If for any reason that fails, it still would display game ready, that's not ideal. In general, you want confirmation-esque messages to be sent from/as part of whatever method is doing the work.

In your displayBoard method, you don't need to have multiple calls to System.out.printlnjust one with concatenation, indentation and the \n newline character suffices.(See example above)

Your checkIfWinner method is something you should consider refactoring to do in a loop.

\$\endgroup\$
  • \$\begingroup\$ It's my pleasure. Good luck on your programming journey. \$\endgroup\$ – Legato Jul 5 '15 at 15:54
  • \$\begingroup\$ I've been conditioned by Markdown to use tabs or four spaces... ;) \$\endgroup\$ – h.j.k. Jul 9 '15 at 2:43

protected by Jamal Mar 1 '17 at 1:27

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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