5
\$\begingroup\$

This is a simple implementation of Connect four in Java. It is working correctly so far. What I haven't done yet are the diagonals check and input validation.

package game;

import java.util.Scanner;
//TODO check for diagonals, input validation

public class Game {
    private char[][] gameBoard;
    private State gameState;
    private Turn turn;
    private int[] possiblePositions;
    private static final int BOARD_HEIGHT = 6;
    private static final int BOARD_WIDTH = 7;

    enum Turn {
        PLAYER_ONE, PLAYER_TWO
    }

    enum State {
        PLAYER_1_WIN, PLAYER_2_WIN, RUNNING
    }

    public Game() {
        gameBoard = new char[BOARD_HEIGHT][BOARD_WIDTH];
        for (int row = 0; row < BOARD_HEIGHT; row++) {
            for (int col = 0; col < BOARD_WIDTH; col++) {
                gameBoard[row][col] = '□';
            }
        }
        turn = Turn.PLAYER_ONE;
        gameState = State.RUNNING;
        possiblePositions = new int[] { 5, 5, 5, 5, 5, 5, 5 };
    }

    public void playerMove(int column) {
        switch (turn) {
        case PLAYER_ONE:
            gameBoard[possiblePositions[column]][column] = '■';
            turn = Turn.PLAYER_TWO;
            break;
        case PLAYER_TWO:
            gameBoard[possiblePositions[column]][column] = '⊠';
            turn = Turn.PLAYER_ONE;
            break;
        }
        possiblePositions[column]--;
    }

    public void printBoard() {
        System.out.println("0 1 2 3 4 5 6");
        for (int row = 0; row < BOARD_HEIGHT; row++) {
            for (int col = 0; col < BOARD_WIDTH; col++) {
                System.out.print(gameBoard[row][col] + " ");
            }
            System.out.println();
        }
    }

    public void checkRowAndCol() {
        int chainLength = 1;
        for (int row = 0; row < BOARD_HEIGHT - 1; row++) {
            for (int col = 0; col < BOARD_WIDTH - 1; col++) {
                if (gameBoard[row][col] == gameBoard[row][col + 1] && gameBoard[row][col] != '□') {
                    ++chainLength;
                    if (chainLength == 4) {
                        switch (gameBoard[row][col]) {
                        case '■':
                            gameState = State.PLAYER_1_WIN;
                            break;
                        case '⊠':
                            gameState = State.PLAYER_2_WIN;
                            break;
                        }
                    }
                } else {
                    chainLength = 1;
                }
            }
            chainLength = 1;
        }

        for (int col = 0; col < BOARD_WIDTH - 1; col++) {
            for (int row = 0; row < BOARD_HEIGHT - 1; row++) {
                if (gameBoard[row][col] == gameBoard[row + 1][col] && gameBoard[row][col] != '□') {
                    ++chainLength;
                    if (chainLength == 4) {
                        switch (gameBoard[row][col]) {
                        case '■':
                            gameState = State.PLAYER_1_WIN;
                            break;
                        case '⊠':
                            gameState = State.PLAYER_2_WIN;
                            break;
                        }
                    }
                } else {
                    chainLength = 1;
                }
            }
            chainLength = 1;
        }
    }

    public void start() {
        Scanner reader = new Scanner(System.in);
        printBoard();
        while (gameState == State.RUNNING) {
            playerMove(reader.nextInt());
            checkRowAndCol();
            printBoard();
        }
        System.out.println(gameState);
        reader.close();
    }
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Magic values

The implementation is mostly fine, except for the widespread magic values that would be better as constants, for example:

private static final char SYMBOL_EMPTY = '□';
private static final char SYMBOL_PLAYER1 = '■';
private static final char SYMBOL_PLAYER2 = '⊠';

Similarly, instead of initializing possiblePositions with new int[] { 5, 5, 5, 5, 5, 5, 5 }, it would be better to derive the correct value from the BOARD_WIDTH and BOARD_HEIGHT constants.

Code duplication

This snippet is duplicated in checkRowAndCol:

if (chainLength == 4) {
    switch (gameBoard[row][col]) {
        case '■':
            gameState = State.PLAYER_1_WIN;
            break;
        case '⊠':
            gameState = State.PLAYER_2_WIN;
            break;
    }
}

It would be better to move this to a helper method.

Unnecessary execution

When a player wins, you could return immediately from checkRowAndCol, no need to continue checking all rows and columns.

The helper method I mentioned in the previous section could return a boolean, with a true value if there is a winner. When the returned value is true, the caller in checkRowAndCol could return immediately.

Code organization

Instead of checkRowAndCol, it would be better to separate to checkRow and checkCol methods. You could still have a checkRowAndCol method that calls the other two, but there's no reason for these two independent aspects to be implemented in the same method.

\$\endgroup\$

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