5
\$\begingroup\$

I am in a computer coding class and we have to make a project for Java.

Is there any way we can better our existing connect four code?

import java.util.*;

public class Connect4 
{

    public static boolean putDisk(char[][] field, int column, char color) {

        if (field[0][column] != ' ')
            return false;


        for (int row = 0; row < 7; ++row) {

            if (field[row][column] != ' ') {

                field[row-1][column] = color;
                return true;
            }
        }

        field[6][column] = color;
        return true;
    }


    private static char getWinnerInRows(char[][] field) {

        for (int row = 0; row < 7; ++row) {
            int count = 0;

            for (int column = 1; column < 7; ++column) {
                if (field[row][column] != ' ' &&
                    field[row][column] == field[row][column-1])
                    ++count;
                else
                    count = 1;


                if (count >= 4) {

                    return field[row][column];
                }
            }
        }

        return ' ';
    }


    private static char getWinnerInColumns(char[][] field) {

        for (int column = 0; column < 7; ++column) {
            int count = 0;

            for (int row = 1; row < 7; ++row) {
                if (field[row][column] != ' ' &&
                    field[row][column] == field[row-1][column])
                    ++count;
                else
                    count = 1;


                if (count >= 4) {

                    return field[row][column];
                }
            }
        }

        return ' ';
    }


    private static char getWinnerInDiagonals(char[][] field) {

        for (int column = 0; column < 7; ++column) {
            int count = 0;

            for (int row = 1; row < 7; ++row) {

                if (column + row >= 7) break;
                if (field[row][column+row] != ' ' &&
                    field[row-1][column + row - 1] == field[row][column+row])
                    ++count;
                else
                    count = 1;
                if (count >= 4) return field[row][column+row];
            }
        }

        for (int row = 0; row < 7; ++row) {
            int count = 0;

            for (int column = 1; column < 7; ++column) {

                if (column + row >= 7) break;
                if (field[row + column][column] != ' ' &&
                    field[row+column - 1][column - 1] == field[row + column][column])
                    ++count;
                else
                    count = 1;
                if (count >= 4) return field[row + column][column];
            }
        }


        for (int column = 0; column < 7; ++column) {
            int count = 0;

            for (int row = 1; row < 7; ++row) {

                if (column - row < 0) break;
                if (field[row][column-row] != ' ' &&
                    field[row - 1][column - row + 1] == field[row][column-row])
                    ++count;
                else
                    count = 1;
                if (count >= 4) return field[row][column-row];
            }
        }


        for (int row = 0; row < 7; ++row) {
            int count = 0;

            for (int column = 5; column >= 0; --column) {

                if (column - row < 0) break;
                if (field[column - row][column] != ' ' &&
                    field[column - row - 1][column + 1] == field[column - row][column])
                    ++count;
                else
                    count = 1;
                if (count >= 4) return field[column - row][column];
            }
        }

        return ' ';
    }

    public static char getWinner(char[][] field) {
        char winner = getWinnerInRows(field);
        if (winner != ' ') return winner;
        winner = getWinnerInColumns(field);
        if (winner != ' ') return winner;
        winner = getWinnerInDiagonals(field);
        if (winner != ' ') return winner;


        for (int i = 0; i < field.length; ++i)
            for (int j = 0; j < field[i].length; ++j)
                if (field[i][j] == ' ') return ' ';

        return 'D';
    }

    public static void printField(char[][] field) {
        for (int row = 0; row < 7; ++row) {
            System.out.print("| ");
            for (int col = 0; col < 7; ++col)
                System.out.print(field[row][col] + "| ");
            System.out.println();
        }


        for (int col = 0; col < 7; ++col)
            System.out.print("---");
        System.out.println();
    }

    public static void main(String[] args) {
        Scanner input = new Scanner(System.in);

        char[][] field = new char[7][7];

        for (int i = 0; i < 7; ++i)
            for (int j = 0; j < 7; ++j)
                field[i][j] = ' ';

        printField(field);

        boolean isRed = true;
        while (true) {
            if (isRed)
                System.out.println("Red's turn!");            
            else 
                System.out.println("Yellow's turn!");
            System.out.print("Choose column (1-7) for a disk:");

            int column = input.nextInt();
            if (column < 1 || column > 7) {
                System.out.println("Column should be from 1 to 7");
                continue;
            }

            if (!putDisk(field, column - 1, isRed ? 'R' : 'Y')) {
                System.out.println("This column is filled! Choose another one.");
                continue;
            }

            printField(field);


            char result = getWinner(field);
            if (result == 'D') {
                System.out.println("It is a draw!");
                break;
            }
            else if (result == 'R') {
                System.out.println("Red win!");
                break;
            }
            else if (result == 'Y') {
                System.out.println("Yellow win!");
                break;
            }

            isRed = !isRed;
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

Code Duplication

Your code has lots of parts which are similar. These can be easily refactored to be the same:

private static char getWinnerInRows(char[][] field) {
    // ..
            if (field[row][column] != ' ' && field[row][column] == field[row][column - 1])
                ++count;
            else
                count = 1;
 // ...
private static char getWinnerInColumns(char[][] field) {
 // ...
            if (field[row][column] != ' ' && field[row][column] == field[row - 1][column])
                ++count;
            else
                count = 1;
 // ...
private static char getWinnerInDiagonals(char[][] field) {
 // ...
            if (column + row >= 7)
                break;
            if (field[row][column + row] != ' ' && field[row - 1][column + row - 1] == field[row][column + row])
                ++count;
            else
                count = 1;
 // some more variants

This code looks almost the same and it is hard do see what the differences are.

We could try to refactor this so that the common logic is in one place and the differences stand out better: To do so we extract the actual index calculations to new local variables that have the same names in all methods:

private static char getWinnerInRows(char[][] field) {
 // ...
            int currentRow = row;
            int currentColumn = column;
            int neighbourRow = row;
            int neighbourColumn = column - 1;
            if (field[currentRow][currentColumn] != ' '
                    && field[currentRow][currentColumn] == field[neighbourRow][neighbourColumn])
                ++count;
            else
                count = 1;
 // ...
private static char getWinnerInColumns(char[][] field) {
 // ...     
            int currentRow = row;
            int currentColumn = column;
            int neighbourRow = row - 1;
            int neighbourColumn = column;
            if (field[currentRow][currentColumn] != ' ' && field[currentRow][currentColumn] == field[neighbourRow][neighbourColumn])
                ++count;
            else
                count = 1;
 // ...
private static char getWinnerInDiagonals(char[][] field) {
 // ...   
            int currentRow = row;
            int currentColumn = column + row;
            int neighbourRow = row - 1;
            int neighbourColumn = column + row - 1;
            if (field[currentRow][currentColumn] != ' ' && field[neighbourRow][neighbourColumn] == field[currentRow][currentColumn])
                ++count;
            else
                count = 1;

In the method getWinnerInDiagonals() we have to switch the order of operands in the second compare to match it with the methods getWinnerInRows() and getWinnerInColumns() (or vice versa):

private static char getWinnerInDiagonals(char[][] field) {
 // ...   
            int currentRow = row;
            int currentColumn = column + row;
            int neighbourRow = row - 1;
            int neighbourColumn = column + row - 1;
            if (field[currentRow][currentColumn] != ' ' && field[currentRow][currentColumn]) == field[neighbourRow][neighbourColumn]
                ++count;
            else
                count = 1;

Do so at all places where this 4 lines exist.

Now select this 4 lines and use your IDEs refactoring extract method.

This will move this lines to a new method and replace all occurences with the call to the new method:

private static int countNeighbourIsSame(char[][] field, int count, int currentRow, int currentColumn,
        int neighbourRow, int neighbourColumn) {
    if (field[currentRow][currentColumn] != ' '
            && field[neighbourRow][neighbourColumn] == field[currentRow][currentColumn])
        ++count;
    else
        count = 1;
    return count;
}

private static char getWinnerInRows(char[][] field) {
 // ...
            int currentRow = row;
            int currentColumn = column;
            int neighbourRow = row;
            int neighbourColumn = column - 1;
            count = countNeighbourIsSame(field, count, currentRow, currentColumn, neighbourRow, neighbourColumn);
 // ...
private static char getWinnerInColumns(char[][] field) {
 // ...     
            int currentRow = row;
            int currentColumn = column;
            int neighbourRow = row - 1;
            int neighbourColumn = column;
            count = countNeighbourIsSame(field, count, currentRow, currentColumn, neighbourRow, neighbourColumn);
 // ...
private static char getWinnerInDiagonals(char[][] field) {
 // ...   
            int currentRow = row;
            int currentColumn = column + row;
            int neighbourRow = row - 1;
            int neighbourColumn = column + row - 1;
            count = countNeighbourIsSame(field, count, currentRow, currentColumn, neighbourRow, neighbourColumn);

Now you still have more lines that you had before, but now these lines contain specific information unique to the method (or block) they are placed it. The actual identical logic is in a single place now. Overall this is much better to read and understand.


But when we have a second look there is still potential to simplify:

private static char getWinnerInRows(char[][] field) {
    for (int row = 0; row < 7; ++row) {
        int count = 0;
        for (int column = 1; column < 7; ++column) {
            int currentRow = row;
            int currentColumn = column;
            int neighbourRow = row;
            int neighbourColumn = column - 1;
            count = countNeighbourIsSame(field, count, currentRow, currentColumn, neighbourRow, neighbourColumn);
            if (count >= 4) {
                return field[row][column];
            }
private static char getWinnerInColumns(char[][] field) {
    for (int column = 0; column < 7; ++column) {
        int count = 0;
        for (int row = 1; row < 7; ++row) {
            int currentRow = row;
            int currentColumn = column;
            int neighbourRow = row - 1;
            int neighbourColumn = column;
            count = countNeighbourIsSame(field, count, currentRow, currentColumn, neighbourRow, neighbourColumn);
            if (count >= 4) {
                return field[row][column];
            }

What if we could define the way the indexes are calculated before the loops? And guess what, we can...

Lets pick getWinnerInRows():

what we need is a new class that encapsulates the actual calculation:

private static char getWinnerInRows(char[][] field) {
    class IndexCalculationRowNeighbour{
    }
    for (int column = 0; column < 7; ++column) {
        int count = 0;
        for (int row = 1; row < 7; ++row) {
            int currentRow = row;
            int currentColumn = column;
            int neighbourRow = row - 1;
            int neighbourColumn = column;

In this new class we need to do the calculation for the four indexes based on the loop variabes:

private static char getWinnerInRows(char[][] field) {
    class IndexCalculationRowNeighbour{
       public int calculateCurrentRow(int row){
          return row;
       }
       public int calculateCurrentColumn(int column){
          return column;
       }
       public int calculateNeighbourRow (int row){
          return row -1;
       }
       public int calculateNeighbourColumn(int column){
          return column;
       }
    }

Now we have to instantiate the class and use it:

private static char getWinnerInRows(char[][] field) {
    class IndexCalculationRowNeighbour{
      // ...
    }
    IndexCalculationRowNeighbour neighbour = new IndexCalculationRowNeighbour();
    for (int column = 0; column < 7; ++column) {
        int count = 0;
        for (int row = 1; row < 7; ++row) {
            int currentRow = neighbour.calculateCurrentRow(row);
            int currentColumn = neighbour.calculateCurrentColumn(column);
            int neighbourRow = neighbour.calculateNeighbourRow(row);
            int neighbourColumn = neighbour.calculateNeighbourColumn(column);

We can copy that whole block to getWinnerInColumns() changing the name of the new class:

private static char getWinnerInColumns(char[][] field) {
    class IndexCalculationColumNeighbour{
       public int calculateCurrentRow(int row){
          return row;
       }
       public int calculateCurrentColumn(int column){
          return column;
       }
       public int calculateNeighbourRow (int row){
          return row;
       }
       public int calculateNeighbourColumn(int column){
          return column -1;
       }
    }
    IndexCalculationColumNeighbour neighbour = new IndexCalculationColumNeighbour();
    for (int column = 0; column < 7; ++column) {
        int count = 0;
        for (int row = 1; row < 7; ++row) {
            int currentRow = neighbour.calculateCurrentRow(row);
            int currentColumn = neighbour.calculateCurrentColumn(column);
            int neighbourRow = neighbour.calculateNeighbourRow(row);
            int neighbourColumn = neighbour.calculateNeighbourColumn(column);

Now we have 2 classes with the same methods. This is a strong singn that they may have a commomn interface;

public class Connect4 {
  interface IndexCalculation {
      int calculateCurrentRow(int row);
      int calculateCurrentColumn(int column);
      int calculateNeighbourRow(int row)
      int calculateNeighbourColumn(int column);
  }
  // ...
private static char getWinnerInRows(char[][] field) {
    class IndexCalculationRowNeighbour() implements IndexCalculation{
      // ...
    }
    IndexCalculation  neighbour = new IndexCalculationRowNeighbour();
  // ...
private static char getWinnerInColumns(char[][] field) {
    class IndexCalculationColumNeighbour implements IndexCalculation{
      // ...
    }
    IndexCalculation  neighbour = new IndexCalculationColumNeighbour();

Now the loops look much more similar.

But they are nested in wrong order because we need to reset the counter before the inner loop. How do we solve this?

We need to uncouple the loop indexes from the array coordinates. We simply pass them both to our new class and the class decides which to use for the calculation:

  interface IndexCalculation {
      int calculateCurrentRow(int outerIndex, int innerIndex);
      int calculateCurrentColumn(int outerIndex, int innerIndex);
      int calculateNeighbourRow(int outerIndex, int innerIndex)
      int calculateNeighbourColumn(int outerIndex, int innerIndex);
  }
  // ...
private static char getWinnerInRows(char[][] field) {
    class IndexCalculationRowNeighbour() implements IndexCalculation{
       @Override
       public int calculateCurrentRow(int outerIndex, int innerIndex){
          return innerIndex;
       }
       @Override
       public int calculateCurrentColumn(int outerIndex, int innerIndex){
          return outerIndex;
       }
       @Override
       public int calculateNeighbourRow (int outerIndex, int innerIndex){
          return  innerIndex -1;
       }
       @Override
       public int calculateNeighbourColumn(int outerIndex, int innerIndex){
          return outerIndex;
       }
  }
  // ...
    IndexCalculation neighbour = new IndexCalculationRowNeighbour();
    for (int outerIndex = 0; outerIndex < 7; ++outerIndex) {
        int count = 0;
        for (int innerIndex = 1; innerIndex < 7; ++innerIndex) {
            int currentRow = neighbour.calculateCurrentRow(outerIndex,innerIndex);
            int currentColumn = neighbour.calculateCurrentColumn(outerIndex,innerIndex);
            int neighbourRow = neighbour.calculateNeighbourRow(outerIndex,innerIndex);
            int neighbourColumn = neighbour.calculateNeighbourColumn(outerIndex,innerIndex);
            if (count >= 4) {
                return field[currentRow][currentColumn];
            }

In getWinnerInColumns() this looks like this:

private static char getWinnerInColumns(char[][] field) {
    class IndexCalculationColumNeighbour() implements IndexCalculation{
       @Override
       public int calculateCurrentRow(int outerIndex, int innerIndex){
          return outerIndex;
       }
       @Override
       public int calculateCurrentColumn(int outerIndex, int innerIndex){
          return innerIndex;
       }
       @Override
       public int calculateNeighbourRow (int outerIndex, int innerIndex){
          return  outerIndex;
       }
       @Override
       public int calculateNeighbourColumn(int outerIndex, int innerIndex){
          return innerIndex -1;
       }
  }
  // ...
    IndexCalculation neighbour = new IndexCalculationColumNeighbour();
    for (int outerIndex = 0; outerIndex < 7; ++outerIndex) {
        int count = 0;
        for (int innerIndex = 1; innerIndex < 7; ++innerIndex) {
            int currentRow = neighbour.calculateCurrentRow(outerIndex,innerIndex);
            int currentColumn = neighbour.calculateCurrentColumn(outerIndex,innerIndex);
            int neighbourRow = neighbour.calculateNeighbourRow(outerIndex,innerIndex);
            int neighbourColumn = neighbour.calculateNeighbourColumn(outerIndex,innerIndex);
            if (count >= 4) {
                return field[currentRow][currentColumn];
            }

As you can see the the loops are now absolute identical.

You can refactor the loops in getWinnerInDiagonals() the same way and then apply the extact method refactoring of your IDE again to copy one of the identical loops to a new method and replace their current occurrences ith a call to that new method.

|improve this answer|||||
\$\endgroup\$

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.