5
\$\begingroup\$

I saw some approaches to the Connect Four game on a site. None of them seemed very OO like, therefore I'd like to present my implementation and request suggestions on how to make this code more readable.

For example, I dislike the "oddball solution" where I get the current positions stone from the field in class ConnectFourOo (playerAt()) because the setPlayer() method is in class Position. It feels more natural to have it there since then I'd reduce the places where the meaning of x and y could be mixed up. But when I move playerAt() to class Position then I find it hard to come up with a good name for it. I'd like to get any comments or suggestions on that and of course anything else which is doubtful in this code.


import java.util.Arrays;
import java.util.Scanner;

public class ConnectFourOo {
    private static final int COLUMN_COUNT = 7;
    private static final int ROW_COUNT = 5;
    private static final int TOP_ROW_INDEX = 0;
    private static final int REQUIRED_CONSECUTIVE_FIELDS = 4;

    private final Stone[][] field;
    private Scanner scanner;
    private Stone currentPlayer;
    private Boundary boundary;

    public ConnectFourOo(Boundary boundary) {
        this.boundary = boundary;
        scanner = new Scanner(System.in);
        // Player NONE transfers to player 1 before first input
        currentPlayer = Stone.NONE;
        field = new Stone[boundary.upper.y][boundary.upper.x];
        // initialize array so we don't have to deal with NULLs
        for (Player[] row : field) {
            Arrays.fill(row, Stone.NONE);
        }
    }

    public static void main(String[] args) {
        new ConnectFourOo(new Boundary(COLUMN_COUNT, ROW_COUNT)).play();
    }

    private void play() {
        Player winner;
        do {
            currentPlayer = currentPlayer.next();
            winner = nextMove();
        } while (Stone.NONE == winner);
        showGame();
        System.out.println("the winner is " + winner.name());
    }

    private Player nextMove() {
        showGame();
        int playersColumn = getUserInput();
        Position pos = putAtColumn(playersColumn);
        return checkWin(pos);
    }

    private int getUserInput() {
        System.out.print(String.format("%s, please select a column (1 - %d):", currentPlayer.name(), boundary.upper.x));
        int playersColumn = scanner.nextInt() - 1;
        return playersColumn;
    }

    private Player checkWin(Position lastMove) {
        // before this move there were no 4 connected stones on the board so we
        // only need to check if this stone completes a line of four in any direction
        for (Direction direction : Direction.values()) {
            Position lastConnected = lastConnectedOwnFieldInOpposite(direction, lastMove);
            int samePlayerCount = countCosecutiveStonesOfCurrentPlayerIn(direction, lastConnected);
            if (REQUIRED_CONSECUTIVE_FIELDS == samePlayerCount)
                return currentPlayer;
        }
        return Stone.NONE;
    }

    private Position lastConnectedOwnFieldInOpposite(Direction direction, Position lastMove) {
        Position next = lastMove;
        Position pos;
        do {
            pos = next;
            next = direction.reverseNext(pos);
        } while (next.isInside(boundary) && currentPlayer == playerAt(next));
        return pos;
    }

    private int countCosecutiveStonesOfCurrentPlayerIn(Direction direction, Position lastMove) {
        int samePlayerCount = 1; // current stone belongs to current player
                                    // already
        Position positionToCheck = lastMove;
        for (int i = samePlayerCount; i < REQUIRED_CONSECUTIVE_FIELDS; i++) {
            positionToCheck = direction.next(positionToCheck);
            if (positionToCheck.isInside(boundary) && playerAt(positionToCheck) == currentPlayer)
                samePlayerCount++;

        }
        return samePlayerCount;
    }

    private Player playerAt(Position positionToCheck) {
        return field[positionToCheck.y][positionToCheck.x];
    }

    private Position putAtColumn(int selectedColumn) {
        Position next = new Position(selectedColumn, TOP_ROW_INDEX);
        Position pos;
        do {
            pos = next;
            next = Direction.DOWN.next(pos);
        } while (next.isInside(boundary) && Stone.NONE == playerAt(next));
        pos.setPlayer(currentPlayer, field);
        return pos;
    }

    private void showGame() {
        System.out.println();
        for (Stone[] row : field) {
            for (Stone field : row) {
                System.out.print(field.toString());
            }
            System.out.println();
        }
        for (int i = 0; i < boundary.upper.x; i++) {
            System.out.print(String.format(" %d ", (1 + i)));
        }
        System.out.println();
    }

}

public interface Player {
    abstract Player next();
    abstract String name();
}

enum Stone implements Player {
    NONE(" ") {
        @Override
        public Stone next() {
            return PLAYER_1;
        }
    },
    PLAYER_1("○") {
        @Override
        public Stone next() {
            return PLAYER_2;
        }
    },
    PLAYER_2("●") {
        @Override
        public Stone next() {
            return PLAYER_1;
        }
    };
    private final String image;

    Stone(String image) {
        this.image = image;

    }

    public String toString() {
        return String.format("[%s]", image);
    }
}

class Position {
    final int x, y;

    public Position(int x, int y) {
        this.x = x;
        this.y = y;
    }

    boolean isInside(Boundary boundary) {
        return x > boundary.lower.x && y > boundary.lower.y && x < boundary.upper.x && y < boundary.upper.y;
    }

    void setPlayer(Player thePlayer, Stone[][] field) {
        field[y][x] = (Stone) thePlayer;
    }
}

enum Direction {
    // Only half of the directions needed since we first find the last connected
    // stone in the opposite direction
    LEFT {
        @Override
        Position next(Position pos) {
            return new Position(pos.x - 1, pos.y);
        }
    },
    DOWN {
        @Override
        Position next(Position pos) {
            return new Position(pos.x, pos.y + 1);
        }
    },
    DOWN_LEFT {
        @Override
        Position next(Position pos) {
            return new Position(pos.x - 1, pos.y + 1);
        }
    },
    DOWN_RIGHT {
        @Override
        Position next(Position pos) {
            return new Position(pos.x + 1, pos.y + 1);
        }
    };
    abstract Position next(Position pos);

    public Position reverseNext(Position pos) {
        Position next = next(pos);
        int reverseX = 2 * pos.x - next.x;
        int reverseY = 2 * pos.y - next.y;
        return new Position(reverseX, reverseY);
    }
}
| improve this question | | | | |
\$\endgroup\$
4
\$\begingroup\$
  • Connect Four is usually played on a 7 × 6 board, not 7 × 5.
  • The ConnectFourOo class currently does two things: managing the state of the game and interacting with the UI. These two things should be in separate classes Board and TextUserInterface, so that you can easily write unit tests for the Board class.
  • Position.setPlayer should be in the Board class, since it modifies the board.
  • Having 5 stones in a row also wins the game. The current code only checks for exactly 4.
  • Stone.NONE.next() should throw an UnsupportedOperationException since it must never be called. At the beginning of the game, it should already be initialized to PLAYER_1.
  • The play method must also work when the game ends in a draw.
| improve this answer | | | | |
\$\endgroup\$
  • \$\begingroup\$ thanks for anwering! "usually played on a 7 × 6 board," since it is easy to change this is not the kind of comment I expect... \$\endgroup\$ – Timothy Truckle Nov 27 '16 at 10:34
  • \$\begingroup\$ "The ConnectFourOo class currently does two things:[...]should be in separate classes Board and TextUserInterface" I'll try that. \$\endgroup\$ – Timothy Truckle Nov 27 '16 at 10:36
  • 1
    \$\begingroup\$ "Position.setPlayer should be in the ConnectFourOo class." with your suggestion of Board class in mind, shouldn't setPlayer go there? \$\endgroup\$ – Timothy Truckle Nov 27 '16 at 10:37
  • \$\begingroup\$ "Having 5 stones in a row also wins the game." is this forbidden by the rules of the game? \$\endgroup\$ – Timothy Truckle Nov 27 '16 at 10:38
  • 1
    \$\begingroup\$ I updated my review in a few places. By the way, I do not consider my review complete, it's just a list of a few points I noticed. So hopefully someone else also answers this. \$\endgroup\$ – Roland Illig Nov 27 '16 at 12:23
4
\$\begingroup\$

Stone implements Player

I find your Player/Stone relationship really confusing. I can see a Player having a name, but next is not something I'd expect something called Player to have. It becomes even more confusing when you introduce the fact that a Stone implements Player. At first glance Player and Stone don't really seem connected. Every Stone has a next operation that returns the next Player. Does that really make any sense?

Personally I'd disconnect the Player and Stone. To me, a Player plays the game and makes a move. They have attributes like a name. A Stone on the other hand represents a move or a counter. It might have a position, a color or an image, but it's not a player.

countCosecutiveStonesOfCurrentPlayerIn

The method name suggests that the method will return the number of consecutive stones for the current player. This is almost true. It will return the number of consecutive stones up to REQUIRED_CONSECUTIVE_FIELDS. This is somewhat confusing when looking at the calling code:

int samePlayerCount = countCosecutiveStonesOfCurrentPlayerIn(direction, lastConnected);
if (REQUIRED_CONSECUTIVE_FIELDS == samePlayerCount)
    return currentPlayer;

You have to know that countCosecutiveStonesOfCurrentPlayerIn can't return more than REQUIRED_CONSECUTIVE_FIELDS otherwise it looks like a bug (as pointed out by @Roland). It's good to optimise your methods to avoid doing unnecessary calculations, however unless you're doing very time sensitive calculations I'd tend to prioritise readability over performance. Having the method return a boolean from a method called playerWinsInDirection(direction,lastConnected) might have been easier to follow.

Up casting

Casting from an interface to a concrete implementation is a flag that your responsibilities might be wrong. You're doing it in Position.setPlayer:

void setPlayer(Player thePlayer, Stone[][] field) {
    field[y][x] = (Stone) thePlayer;
}

You pass in a Player (which could actually be implemented by any class) and cast it straight to a stone, because that's what your field needs. This is a good indication that something is wrong. It could be the method is in the wrong place, that the field is at the wrong level of abstraction, or simply that the method should really be setStone and take a stone rather than a player.

Direction

Whilst you don't need all of the directions in your Direction enum, it's natural to expect something called Direction with a DOWN to have an UP, without it it feels incomplete. reverseNext is also somewhat confusing member function for it a Direction and I can't help but feel like it could have been modelled by simply adding the missing directions...

Alternative

I've had a go below at restructuring some of your code to share some of the responsibilities about. I haven't spent a huge amount of time on it, so there are going to be some outstanding issues however I think it offers an alternative perspective on where things could sit.

ConnectFourOo

The main entry point for the application is responsible for constructing other objects and keeping the game moving. It knows who the current/next player is and is currently responsible for user input.

import java.util.Scanner;

public class ConnectFourOo {
    private static final int COLUMN_COUNT = 7;
    private static final int ROW_COUNT = 5;

    private Scanner scanner;
    private int currentPlayer;
    Player[] players;
    int numberOfPlayers;
    Board board;
    Boundary boundary;
    private Connect4WinChecker winChecker;

    public ConnectFourOo(Board board, Boundary boundary) {
        scanner = new Scanner(System.in);
        // Player NONE transfers to player 1 before first input
        currentPlayer = 0;
        numberOfPlayers=2;
        players = new Player[2];
        players[0] = new Player("Player_1", Stone.RED);
        players[1] = new Player("Player_2", Stone.YELLOW);
        this.board = board;
        this.boundary = boundary;
        winChecker = new Connect4WinChecker(this.board);
    }

    public static void main(String[] args) {
        Boundary boundary = new Boundary(COLUMN_COUNT, ROW_COUNT);
        new ConnectFourOo(new Board(boundary), boundary).play();
    }

    private void play() {
        Player winner = null;
        while(winner == null){
            winner = nextMove();
            currentPlayer = (currentPlayer + 1) % 2;
        } 
        board.show();
        System.out.println("the winner is " + winner.name());
    }

    private Player nextMove() {
        board.show();
        int playersColumn = getUserInput();
        Position pos = board.putAtColumn(playersColumn, players[currentPlayer].getStone());
        return winChecker.winningMove(pos) ? players[currentPlayer] : null;
    }

    private int getUserInput() {
        System.out.print(String.format("%s, please select a column (1 - %d):", players[currentPlayer].name(), boundary.upper.x));
        int playersColumn = scanner.nextInt() - 1;
        return playersColumn;
    }
}

Player

Each player has a name and a stone which determines what token they play when it is their move. These values are set by the ConnectFourOo on construction.

public class Player {
    String name;
    Stone stone;

    public Player(String name, Stone stone) {
        this.name = name;
        this.stone = stone;
    }
    Player next() { return this; }
    String name() {
        return name;
    }

    public Stone getStone() { return stone; }
}

Stone

Simply represents a stone that can be played on the board.

enum Stone {
    NONE(" "),
    RED("X"),
    YELLOW("O");

    private final String image;

    Stone(String image) {
        this.image = image;

    }

    public String toString() {
        return String.format("[%s]", image);
    }
}

Board

Represents the game board. It is responsible for storing all of the stones that have been played and has a public method putAtColumn for putting a stone in the next available slot in a given column.

This code has some issues including, putAtColumn doesn't have a way of indicating that an invalid move has been selected (it could throw an exception). show writes directly to the console. It feels like this should be done a different way, however it works at the moment. The board could use an isFull method that could be used to indicate a draw condition.

import java.util.Arrays;

public class Board {
    private static final int TOP_ROW_INDEX = 0;

    private Boundary boundary;
    private final Stone[][] field;


    public Board(Boundary boundary) {
        this.boundary = boundary;        
        field = new Stone[boundary.upper.y][boundary.upper.x];
        for (Stone[] row : field) {
            Arrays.fill(row, Stone.NONE);
        }
    }

    public Stone stoneAt(Position positionToCheck) {
        return field[positionToCheck.y][positionToCheck.x];
    }    

    public Position putAtColumn(int selectedColumn, Stone stone) {
        Position next = new Position(selectedColumn, TOP_ROW_INDEX);
        Position pos;
        do {
            pos = next;
            next = Direction.DOWN.next(pos);
        } while (isInside(next) && Stone.NONE == stoneAt(next));
        setStone(stone, pos);
        return pos;
    }

    boolean isInside(Position pos) {
        return pos.x > boundary.lower.x && pos.y > boundary.lower.y && pos.x < boundary.upper.x && pos.y < boundary.upper.y;
    }

    void setStone(Stone stone, Position pos) {
        field[pos.y][pos.x] = stone;
    }       

    public void show() {
        System.out.println();
        for (Stone[] row : field) {
            for (Stone field : row) {
                System.out.print(field.toString());
            }
            System.out.println();
        }
        for (int i = 0; i < boundary.upper.x; i++) {
            System.out.print(String.format(" %d ", (1 + i)));
        }
        System.out.println();
    }

}

Connect4WinChecker

Simply responsible for checking if the last move was a winning move. I've moved this outside of the board, because it feels like it's more related to the actual game rules than the board, which could be used for other downfall games (connect 3, connection 5 etc). I'm not overly sold on the name.

public class Connect4WinChecker {
    private static final int REQUIRED_CONSECUTIVE_FIELDS = 4;

    Board board;

    public Connect4WinChecker(Board board) {
        this.board = board;
    }

    public boolean winningMove(Position lastMove) {
        Stone lastStone = board.stoneAt(lastMove);
        for (Direction direction : Direction.values()) {
            Position lastConnected = lastConnectedOwnFieldInOpposite(direction, lastMove, lastStone);
            if(playerWinsInDirection(direction, lastConnected, lastStone))
                return true;
        }
        return false;
    }    

    private Position lastConnectedOwnFieldInOpposite(Direction direction, Position lastMove, Stone playedStone) {
        Position next = lastMove;
        Position pos;
        do {
            pos = next;
            next = direction.reverseNext(pos);
        } while (board.isInside(next) && playedStone == board.stoneAt(next));
        return pos;
    }

    private boolean playerWinsInDirection(Direction direction, Position lastMove, Stone playedStone) {
        int samePlayerCount = 1; 
        Position positionToCheck = lastMove;
        for (int i = samePlayerCount; i < REQUIRED_CONSECUTIVE_FIELDS; i++) {
            positionToCheck = direction.next(positionToCheck);
            if (board.isInside(positionToCheck) && board.stoneAt(positionToCheck) == playedStone)
                samePlayerCount++;

        }
        return samePlayerCount == REQUIRED_CONSECUTIVE_FIELDS;
    }

}

Position

Simply represents an x/y location. Knows nothing about boundaries/players.

class Position {
    final int x, y;

    public Position(int x, int y) {
        this.x = x;
        this.y = y;
    }
}

Direction

I didn't modify the Direction enum, because I think it's too coupled to your implementation to refactor without modifying your logic.

enum Direction {
    // Only half of the directions needed since we first find the last connected
    // stone in the opposite direction
    LEFT {
        @Override
        Position next(Position pos) {
            return new Position(pos.x - 1, pos.y);
        }
    },
    DOWN {
        @Override
        Position next(Position pos) {
            return new Position(pos.x, pos.y + 1);
        }
    },
    DOWN_LEFT {
        @Override
        Position next(Position pos) {
            return new Position(pos.x - 1, pos.y + 1);
        }
    },
    DOWN_RIGHT {
        @Override
        Position next(Position pos) {
            return new Position(pos.x + 1, pos.y + 1);
        }
    };
    abstract Position next(Position pos);

    public Position reverseNext(Position pos) {
        Position next = next(pos);
        int reverseX = 2 * pos.x - next.x;
        int reverseY = 2 * pos.y - next.y;
        return new Position(reverseX, reverseY);
    }
}
| improve this answer | | | | |
\$\endgroup\$
  • \$\begingroup\$ "Stone implements Player" I chose this because I didn't want a separate Stone class having a player as single property. The consequence of this would be to have either separate Stone and Player enums with almost the same purpose or each stone at the field would be a separate Stone object which would make the "is same" check a little harder. But if it improves readability I agree that this would be worth it. \$\endgroup\$ – Timothy Truckle Nov 27 '16 at 23:17
  • \$\begingroup\$ "countCosecutiveStonesOfCurrentPlayerIn" the name you suggested playerWinsInDirection is great. Thanks! \$\endgroup\$ – Timothy Truckle Nov 27 '16 at 23:18
  • \$\begingroup\$ "Up casting" Your suggestion of a separate Stone class would really improve this. Thanks again. \$\endgroup\$ – Timothy Truckle Nov 27 '16 at 23:20
  • \$\begingroup\$ "Direction: it it feels incomplete, reverseNext is also somewhat confusing" The point is: when I have enum constants for all directions I I have 3 possibilities to get the reverse step: 1) keep the generic calculation: The reader may ask why I don't reuse the existing enum valuess. 2) find the opposite direction by its ordinal: this makes assumptions about the enum which may change. 3) have each direction return its opposite: this would make the enum twice as big (fourth as big as with my approach) \$\endgroup\$ – Timothy Truckle Nov 27 '16 at 23:30
  • 1
    \$\begingroup\$ @TimothyTruckle I've added an alternative refactoring of your code, that shuffles some of the responsibilities around. It's not a complete overhaul, but may be useful. \$\endgroup\$ – forsvarir Nov 28 '16 at 11:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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