Connect Four best way to get diagonals

Question

I've checked many classes that implement the major diagonal cases but I didn't find efficient one. I tried to implement major diagonal on the path that I used with rows and columns but it's not completed yet.

Here is what I've done so far:

public class Game_ConnectFour_7_20 {

    public static void startGame() {

        String[][] board = initArray();

        printBoard(board);

        displayGame(board);
    }

    private static void printBoard(String[][] board) {
        for (int row = 0; row < board.length; row++) {
            for (int column = 0; column < board[row].length; column++) {
                System.out.print("|" + board[row][column]);
            }
            System.out.println("|");
        }
    }

    private static void displayGame(String[][] board) {
        Scanner input = new Scanner(System.in);
        String currentPlayer = "R", playerName = "Red";
        for (int row = board.length - 1; row > 0; row--) {
            int currentRow = row;
            for (int column = 0; column < board[row].length; column++) {
                currentRow = board.length - 1;
                currentPlayer = (currentPlayer.equals("R") ? "Y" : "R");
                playerName = (playerName.equals("Red") ? "Yellow" : "Red");
                System.out.println("Drop a " + playerName + " disk at column (0-6)");
                int gameStep = input.nextInt();

                while (!board[currentRow][gameStep].equals(" ")) {
                    currentRow--;
                }
                board[currentRow][gameStep] = currentPlayer;
                printBoard(board);
                if (isConsecutiveFour(board, currentPlayer, currentRow, gameStep))
                    System.out.println("Winner");
            }
        }
    }

    private static boolean isConsecutiveFour(String[][] list, String currentPlayer, int row, int column) {
        return isConsecutiveFourRow(list, row, currentPlayer) ||
                isConsecutiveFourColumn(list, column, currentPlayer) ||
                isConsecutiveFourMajor(list, currentPlayer, row, column);
    }



    private static boolean isConsecutiveFourColumn(String[][] list, int currentColumn, String currentValue) {

        int count = 0;
        for (int row = 0; row < list.length; row++) {
            if (list[row][currentColumn].equals(currentValue))
                count++;
            else
                count = 0;
        }
        return count == 4;
    }

    private static boolean isConsecutiveFourRow(String[][] list, int currentRow, String currentValue) {

        int count = 0;
        for (int column = 0; column < list[currentRow].length; column++) {
            if (count == 4)
                break;
            if (list[currentRow][column].equals(currentValue))
                count++;
            else
                count = 0;
        }
        return count == 4;
    }



    private static String[][] initArray() {

        String[][] arr = new String[6][7];
        for (int row = 0; row < arr.length; row++) {
            for (int column = 0; column < arr[row].length; column++) {
                arr[row][column] = " ";
            }
        }
        return arr;
    }
}

The part that I want to maintain:

private static boolean isConsecutiveFourMajor(String[][] list, String currentValue, int currentRow, int currentColumn) {

        int countMajorDiagonal = 0, countMajorSubDiagonal = 0;


        for (int majorDiagonal = currentRow + 1; majorDiagonal < list.length; majorDiagonal++) {
            if (countMajorDiagonal == 4)
                break;
            if (list[majorDiagonal][majorDiagonal].equals(currentValue))
                countMajorDiagonal++;
            else countMajorDiagonal = 0;
        }

//        for (int majorDiagonal = 0; majorDiagonal < list.length; majorDiagonal++) {
//            for (int j = majorDiagonal; j < majorDiagonal + 1; j++) {
//
//                if (countMajorDiagonal == 4)
//                    break;
//                if (list[majorDiagonal][j].equals(currentValue))
//                    countMajorDiagonal++;
//                else countMajorDiagonal = 0;
//            }
//        }

        for (int subDiagonal = 0; subDiagonal < list.length; subDiagonal++) {
            if (countMajorSubDiagonal == 4)
                break;
            if (list[subDiagonal][(list.length - subDiagonal) - 1].equals(currentValue))
                countMajorSubDiagonal++;
            else countMajorSubDiagonal = 0;
        }

        return countMajorDiagonal == 4 || countMajorSubDiagonal == 4;
    }

Answer 1

Thanks for sharing your code.

OOP

Your code is a procedural approach to the problem.

There is nothing wrong with procedural approaches in general, but Java is an object oriented (OO) programming language and if you want to become a good Java programmer then you should start solving problems in an OO way.

OOP doesn't mean to "split up" code into random classes.

The ultimate goal of OOP is to reduce code duplication, improve readability and support reuse as well as extending the code.

Doing OOP means that you follow certain principles which are (among others):

• information hiding / encapsulation
• single responsibility
• separation of concerns
• KISS (Keep it simple (and) stupid.)
• DRY (Don't repeat yourself.)
• "Tell! Don't ask."
• Law of demeter ("Don't talk to strangers!")

Your code would benefit from an additional class Player that holds both, the players name and its "coin mark". That would reduce the switching of the player to a oneliner.

Naming

Finding good names is the hardest part in programming, so always take your time to think about the names of your identifiers.

Naming Conventions

Please read (and follow) the Java Naming Conventions

Eg. you class name has underscores but you should separate words in your identifiers by camel case only.

misleading naming

Some of your methods have somewhat misleading names, eg. displayGame(board). This method really implements the game loopwhich should be expressed in the methods name.

Magic numbers

your code has some magic numbers. This are literal values with a special meaning like here:

        if (count == 4)
            break;
        // ...
    return count == 4;

You should introduce constants with meaningful names:

 private static final int CONSECUTIVE_COINS_TO_WIN = 4;
        // ...
        if (count == CONSECUTIVE_COINS_TO_WIN)
            break;
        // ...
    return count == CONSECUTIVE_COINS_TO_WIN;

Single Responsibility/Separation of Concerns

In your excample you mix business logic with user interaction like this:

      for (int column = 0; column < board[row].length; column++) {
            currentRow = board.length - 1;
            currentPlayer = (currentPlayer.equals("R") ? "Y" : "R");
            playerName = (playerName.equals("Red") ? "Yellow" : "Red");
            System.out.println("Drop a " + playerName + " disk at column (0-6)");
            int gameStep = input.nextInt();

            while (!board[currentRow][gameStep].equals(" ")) {
                currentRow--;    currentRow--;
            }
            board[currentRow][gameStep] = currentPlayer;
            printBoard(board);
            if (isConsecutiveFour(board, currentPlayer, currentRow, gameStep))
                System.out.println("Winner");
        }

You should separate the different responsibilities into different methods (and maybe into different classes) like this:

      for (int column = 0; column < board[row].length; column++) {
            Player currentPlayer= switchUser(); // player is a custom object rather than a string...
            int selectedColumn= userInterface.askForNextColumn(currentPlayer);
            dropCoinAtColumn(selectedColumn);
            userInterface.printBoard(board);
            if (isConsecutiveFour(board, currentPlayer, currentRow, gameStep))
               userInterface.reportWinOf(currentPlayer);
        }

define variables as late as possible/ minimize scope

You define your variable currentRow before the inner for loop, but you only use it inside the loop when dropping the coin. This makes it hard to move the coin drop code to its own method. Your IDEs automated refactoring extract method will introduce both, an extra parameter and an unneeded return value.

So always declare variables just before their first use.