Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I'm currently working on a Tic Tac Toe project to improve my programming skills, and I'm simultaneously learning Git.

This is my GitHub page for my Tic Tac Toe project. I will post the code below so you don't have to go to GitHub. But if you could give me some small feedback whether I have pushed/committed things in the correct way, that'd be nice.

Anyway, before I post my code, here is what I have in mind - you might want to consider this when answering (I would also appreciate if you can give some feedback to this approach, and maybe some ideas):

I decided to start a Tic Tac Toe project, and see it through. Here is what I'm currently thinking of (the various stages may be vague for now, but that's OK):

  • start with a simple console program, for two players
  • create an artificial intelligence (AI), which makes random moves (easy to play against)
    • add a game menu which offers the option of single- and multiplayer
    • add a stronger AI using the Minimax algorithm and adjust the menu accordingly
  • make a simple GUI version
  • improve the GUI in various ways (current ideas - no idea how hard they are to implement: )
    • add multiple themes
    • add animations for the moves
    • add sound
    • switch background color (light/dark)

This would be my initial goal. I have other ideas, which might come into play as well, as in:

  • keep track of statistics
  • make it playable on the web

And here is the code (5 classes in total, as of now):

package game;

public enum Sign {
  X, O;
}

package game;

public class Main {
    public static void main(String[] args) {
        // play Tic Tac Toe
        Game game = new Game();
        game.playGame();
    }
}

package game;

public class Player {
    private String name;
    private Sign sign;

    public Player(String name) {
        this.name = name;
    }

    public void setSign(Sign sign) {
        this.sign = sign;
    }

    public Sign getSign() {
        return sign;
    }

    @Override
    public String toString() {
        return name;
    }
}

package game;

public class Board {
    private String[][] board;

    public Board() {
        init();
    }

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

    private void init() {
        board = new String[3][3];
        int count = 1;

        for(int i=0; i < board.length; ++i) {
            for(int j=0; j < board[i].length; ++j) {
                // set the field positions, from 1 to 9
                board[i][j] = "" + count++;
            }
        }
    }

    public boolean isFull() {
        boolean isFull = true;

        for(int i=0; i < board.length; ++i) 
            for(int j=0; j < board.length; ++j)
                // still contains an unmarked field (indicated by a number)
                if("123456789".contains(board[i][j]))
                    isFull = false;

        return isFull;
    }

    /**
     * print the string represantion of the current board
     */
    public void printBoard() {
        System.out.println(this);
    }

    @Override
    public String toString() {
        StringBuilder sb = new StringBuilder();
        int formatSize = 0;

        for(int i=0; i < board.length; ++i) {
            for(int j=0; j < board[i].length; ++j) {
                sb.append(board[i][j] + " | ");
            }

            // #_|_#_|_#_|_ - delete the last two blanks + |
            sb.delete(sb.length()-3, sb.length());
            // determines how many "-" to draw horizontally to make it look more aligned 
            formatSize = (formatSize == 0) ? sb.length() : formatSize;

            if(i < board.length-1) {
                sb.append(System.lineSeparator());
                // http://stackoverflow.com/questions/1235179/simple-way-to-repeat-a-string-in-java
                sb.append(new String(new char[formatSize]).replace("\0", "-"));
                sb.append(System.lineSeparator());
            }
        } 

        return sb.toString();
    }
}

package game;

import java.util.*;

public class Game {
    private Board board;
    private Player player1, player2;
    boolean isTurnP1, hasThreeInARow;
    private List<String> usedPositions;

    public Game() {
        initGame();
    }

    @SuppressWarnings("resource")
    private void initGame() {
        board = new Board();

        Scanner scan = new Scanner(System.in);
        System.out.print("Player 1 enter your name: ");
        player1 = new Player(scan.nextLine());
        player1.setSign(Sign.X);
        System.out.print("Player 2 enter your name: ");
        player2 = new Player(scan.nextLine());
        player2.setSign(Sign.O);

        usedPositions = new LinkedList<>();
    }


    /**
     * play a Tic-Tac-Toe game 
     */
    public void playGame() {
        isTurnP1 = isP1start();

        while(!board.isFull()) {
            callForTurn();
            printBoard();
            String nextTurn = makeMove();
            setMark(nextTurn);
            hasThreeInARow = isWin();
            if(hasThreeInARow)
                break;
            isTurnP1 = !isTurnP1; // changeTurn()
        }

        System.out.println();
        printBoard();

        if(hasThreeInARow)
            declareWinner();
        else
            System.out.println("draw");
    }

    // what follows are auxiliary methods for playGame(); to make playGame() more readable

    private boolean isP1start() {
        return new Random().nextInt(2) == 0;
    }

    private void callForTurn() {
        System.out.println();
        if(isTurnP1)
            System.out.printf("%s's turn (%s)%n", player1, player1.getSign());
        else
            System.out.printf("%s's turn (%s)%n", player2, player1.getSign());
    }

    private void printBoard() {
        System.out.println("=========");
        board.printBoard();
        System.out.println("=========");
    }

    @SuppressWarnings("resource")
    private String makeMove() {
        System.out.print("Your move: ");
        Scanner scan = new Scanner(System.in);
        String move = scan.next();
        while(move.length() != 1 || !"123456789".contains(move) || usedPositions.contains(move)) {
            System.out.print("Faulty input, try again: ");
            move = scan.next();
        }

        usedPositions.add(move);

        return move;
    }

    private void setMark(String move) {
        for(int i=0; i < board.getBoard().length; ++i) {
            for(int j=0; j < board.getBoard()[i].length; ++j) {
                if(board.getBoard()[i][j].equals(move)) {
                    board.getBoard()[i][j] = isTurnP1 ? player1.getSign() + "" 
                                                      : player2.getSign() + "";
                } 
            }
        }
    }

    private boolean isWin() {
        String[][] field = board.getBoard();

        boolean horizontal = 
                field[0][0].equals(field[0][1]) && field[0][1].equals(field[0][2])
                || ( field[1][0].equals(field[1][1]) && field[1][1].equals(field[1][2]) )
                || ( field[2][0].equals(field[2][1]) && field[2][1].equals(field[2][2]) );

        boolean vertical =
                field[0][0].equals(field[1][0]) && field[1][0].equals(field[2][0])
                || field[0][1].equals(field[1][1]) && field[1][1].equals(field[2][1])
                || field[0][2].equals(field[1][2]) && field[1][2].equals(field[2][2]);

        boolean diagonal = 
                field[0][0].equals(field[1][1]) && field[1][1].equals(field[2][2])
                || field[0][2].equals(field[1][1]) && field[1][1].equals(field[2][0]);

        return horizontal || vertical || diagonal;
    }

    private void declareWinner() {
        if(isTurnP1)
            System.out.printf("%s (%s) won%n", player1, player1.getSign());
        else
            System.out.printf("%s (%s) won%n", player2, player2.getSign());
    }
}

In the Game.isWin() I decided to hardcode the logic that determines the winner. I thought that solving this in another way would overcomplicate things for a simple game as Tic Tac Toe.

Anyway, I'm open for any kind of feedback. If there are parts that could have been handled better using Java 8, I'd like to hear that too, as I'm still in the Java 5/7 world (couldn't find any appealing Java 8 tutorial yet).

share|improve this question
1  
I would definitely look at Java 8 - the changes are massive and as such learning Java 7 is almost pointless as a huge amount (collections for example) is superseded in Java 8. Really; Java 8! – Boris the Spider Jan 12 at 17:15
    
@BoristheSpider Do you have a site you could recommend? There are the Java Tutorials from Oracle, but I really don't like those walls of text. It's at best a reference source for me. I wasn't too fond of the other tutorials I found either, but I guess there is no helping it. – Beko Jan 12 at 18:38
up vote 6 down vote accepted

Your plan sounds good, I don't have much to add to it. What you may want to do is create a (UML) class diagram to help you plan out the structure of your code (even though it's a bit of extra work, it's often really helpful to get an overview first, instead of coding something, realizing that something is missing, and just putting it into the first place that fits).

Structure

You already have a vision of where you want to end up, which is good, as it can help you learn to plan ahead when designing your code, and make it easily extendable.

However, right now, your code is not all that extendable. For example, you have a plan to accept input from different sources than the command line (in your case a GUI and an AI).

If you want to add that later on, you would have to change your Game class. But the Game class shouldn't actually care where input comes from. It should manage the game logic, nothing more. Currently, it's responsible to read input, print output, create the players, etc, making it more difficult to apply changes.

So a first step would be to extract that functionality. Create a Input and Output interface, and then concrete implementations of them (in your case that would be ConsoleInput and ConsoleOutput). Methods may be something like Move getMove(Board board, int decissionTime), void sendMessage(String message, Severity severity), void showBoard(Board board), void showMove(Move move), etc.

You then want to pass those concrete implementations on to your Game class via the constructor. That way, you can later just pass different implementations when you want to implement an AI or a GUI.

You might also want to add some more classes, such as a Move class to help you handle this.

When doing this restructuring, always keep in mind what functionality you might want to add in the future. Would it be easy to add animation with your current structure? How about an AI? etc.

  • why have usedPositions in Game? Couldn't you just check the Board if it contains something at that position? It would be faster and cleaner, as it's the job of the board to hold the board state, not the job of the game.
  • Same with isWin. The Game class should not care about the internal structure of Board (it makes your code unnecessarily complex; also, what if the structure changes?).
  • if you can make classes immutable, do so. For example, there is no reason to have setters in your Player class, as the sign of a player will rarely change.
  • what's the difference between printBoard and toString? There doesn't seem to be one, so just remove printBoard.

Misc

  • Your formatting is mostly good, but could use some improvement (regarding whitespace, use of curly brackets, etc). Just use any IDE to fix formatting, and look at the hints of the Lint program it runs to fix common mistakes (such as missing curly brackets).
share|improve this answer
1  
Thanks. Yeah, I'm struggeling quite a lot with "how to program like an OOP programmer". Probably because I'm just that book knowledge guy whose sole programming experience is to do exercises (and that's been a while now). This is probably why I can't program code that is extandable - since I don't know what's to come. From that perspective this answer was really helpful to me, and I hope I can implement these suggestions. You also sugggested improvements to the current code and took my "gameplan" into consideration, hence I accepted your answer. [end long comment] – Beko Jan 12 at 12:59

Sign is good, main is good, Player is okay, I don't like Board, and Game is a mess. Relatively speaking. It's pretty naturally written, but you seem to be lacking some tools. Specifically, the idea of using object references as more than just storage of data, and "tricks" like using arithmetic in accessing data structures.

First, learn to remove duplication.

private void declareWinner() {
    if(isTurnP1)
        System.out.printf("%s (%s) won%n", player1, player1.getSign());
    else
        System.out.printf("%s (%s) won%n", player2, player2.getSign());
}

This ought to just take a player argument.

Then you can do this:

private void declareWinner(Player winner) {
    System.out.printf("%s (%s) won%n", winner, winner.getSign());
}

Second, I think you had some trouble separating the view and the model - you refer to board cells internally as Strings, and the result is having to do things like this:

private void setMark(String move) {
    for(int i=0; i < board.getBoard().length; ++i) {
        for(int j=0; j < board.getBoard()[i].length; ++j) {
            if(board.getBoard()[i][j].equals(move)) {
                board.getBoard()[i][j] = isTurnP1 ? player1.getSign() + "" 
                                                  : player2.getSign() + "";
            } 
        }
    }
}

There's two ways I quickly see about this; First is just making a list instead of a 2d array. That way you can parse the input as a number and jump to the position in the board.

The other, involves numbers as well, but you just leave it as a 2d array, and use the modulo % operator instead. It gives you the remainder of a division, so 8 mod 3 is 2. Since you start at 1 instead of 0, you would need to do board.getBoard()[(move-1)/3][(move-1)%3] to get the tile you wanted to get at.

Look up Integer.parseInt. Maybe handle it in the scanner, but beware of format exceptions.

private void callForTurn() {
    System.out.println();
    if(isTurnP1)
        System.out.printf("%s's turn (%s)%n", player1, player1.getSign());
    else
        System.out.printf("%s's turn (%s)%n", player2, player1.getSign());
}

Duplication again, take a player argument to resolve to

private void callForTurn(Player player) {
    System.out.println();
    System.out.printf("%s's turn (%s)%n", player, player.getSign());
}

I think if you were to replace your isTurnP1 variable with a Player variable instead (activePlayer?), a lot of the fixes in Game would become easier to apply.

share|improve this answer
    
Thanks for the fast input. I'll consider all of this. As for the "separation of the view and the model" and its solution you suggest. I see the problem and I understand the idea of using a list instead, but the latter option seems more appealing to me as I'd have to refactor a little less. However, I don't understand how exactly I should use %there to improve the code. – Beko Jan 12 at 10:35
1  
@Beko board[(move-1)/3][(move-1)%3] gives you the index you were looking for with your for-loop, provided move is int. All you gotta check is if it's already taken. You can use Integer.parseInt to convert String to int. Alternatively, make use of the Collections and use a Map<String, Tile> thing of sorts, so you can just directly access the tile from the string. – Pimgd Jan 12 at 10:44

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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