The Game of Life that scored 100%

Question

Note: this is from a COMPLETED course. This is not a ploy to get help with homework, as I have already received a 100% on this assignment (last year). I just want to become a neater programmer. Can you please provide me with some stylistic feedback? Note, the assignment did not allow for the use of objects at the time and the dat files contained X's and .'s. X's = life, .'s = absence of life. It was an exercise to provide us practice with multi-dimensional arrays.

package java_labs.GameOfLife;

import java.util.Scanner;
import java.io.*;

public class GameOfLife {
    final static int M = 25;
    final static int N = 75;

    public static void main(String[] args) {
        char[][] oldBoard = new char[M + 2][N + 2];
        char[][] newBoard = new char[M + 2][N + 2];
        char[][] tempBoard = new char[M + 2][N + 2];

        buildBoardFromFile(oldBoard, newBoard, tempBoard);

        int generationCount = 0;

        printBoard(oldBoard, generationCount); /* prints Generation #0 every time*/

        boolean continueGame = true;
        while (continueGame) {
            if (isEmpty(oldBoard)) {
                continueGame = false;
            } else {

                newBoard = getNextGeneration(oldBoard, newBoard);

                if (isGenerationChanged(oldBoard, newBoard)) {
                    tempBoard = copyArray(newBoard, tempBoard);
                    Scanner input = new Scanner(System.in);
                    String inputValue = " ";

                    while ((!(inputValue.equalsIgnoreCase("Y")))
                            && (!(inputValue.equalsIgnoreCase("Q")))) {
                        System.out.print("\nWould you like to see the next generation?\n"
                                        + "Enter 'Y' for yes or 'Q' to quit: ");

                        inputValue = input.next();

                        if (inputValue.equalsIgnoreCase("Q")) {
                            continueGame = false;
                            System.out.println("\nYou opted to quit. "
                                    + "Game Over!\n");
                        } else if (inputValue.equalsIgnoreCase("Y")) {
                            generationCount++;
                            printBoard(newBoard, generationCount);
                            oldBoard = tempBoard;
                        } else {
                            System.out.println("\nInvalid option. Try again.");
                        }
                    }
                } else {
                    continueGame = false;
                    generationCount++;

                    System.out.println("\n\nGAME OVER! Generation #"
                                    + (generationCount)
                                    + " is the same as Generation #"
                                    + (generationCount - 1)
                                    + ".\nBelow is proof that Generation #"
                                    + (generationCount)
                                    + " is the same as Generation #"
                                    + (generationCount - 1)
                                    + "\nfor your reference:\n");

                    printBoard(newBoard, generationCount);

                    System.out.println("\n\n\nYou will now exit the program!\n\n\n");
                }
            }
        }
    }

    static void buildBoardFromFile(char[][] oldBoard, char[][] newBoard,
            char[][] tempBoard) {

        boolean inputInvalid = true;
        Scanner fileReader = null;
        File file = null;
        String line = "";
        boolean invalidNumberOfRowCol = false;

        while (inputInvalid) {
            Scanner consoleReader = new Scanner(System.in);
            System.out.print("\nWhich file do you want to open?");
            String filename = consoleReader.next();
            file = new File(filename);

            try {
                fileReader = new Scanner(file);
                inputInvalid = false;
            }

            catch (Exception e) {
                System.out.println("\nError: File " + file
                        + " does not exist! Try again:");
            }
        }

        for (int row = 0; row < M; row++) {
            try {
                line = fileReader.nextLine();
            }/* Accounts for files with too few rows */

            catch (java.util.NoSuchElementException err) {
                System.out.println("\nError: " + file
                        + " is not compatible with"
                        + " this program! Try adding " + (M - (row))
                        + " rows to\n" + file + " to fix this issue.");
                buildBoardFromFile(oldBoard, newBoard, tempBoard);
                break;
            }

            for (int col = 0; col < N; col++) {
                /* Added this code to account for files with too few columns */
                try {
                    oldBoard[row + 1][col + 1] = line.charAt(col);
                }

                catch (StringIndexOutOfBoundsException err) {
                    System.out.println("\nError: " + file
                            + " is not compatible with this program!"
                            + " Try adding " + (N - line.length())
                            + " characters to\nrow #" + (row + 1) + " in "
                            + file + " to fix this issue.");
                    invalidNumberOfRowCol = true;
                    break;
                }
            }
        }/*
         * Note this program will ignore data in file greater outside the
         * maximum column size and row size. For example, if a row in the .dat
         * file contains 76 characters and the maximum board size is 75, the
         * 76th character in that row is ignored by the program's for loop. This
         * assignment did not ask the user to address this issue. I did create
         * additional error checking functionality outside the scope of the
         * assignment for practice. See above.
         */

        if (invalidNumberOfRowCol)
            buildBoardFromFile(oldBoard, newBoard, tempBoard);

        setBoarder(oldBoard, newBoard, tempBoard);
    }

    /* Note boarder will always be . and will not change */
    static void setBoarder(char[][] oldBoard, char[][] newBoard,
            char[][] tempBoard) {
        for (int row = 0; row <= M + 1; row++) {
            for (int col = 0; col <= N + 1; col++) {
                if (row == 0 || col == 0 || row == M + 1 || col == N + 1) {
                    oldBoard[row][col] = '.';
                    newBoard[row][col] = '.';
                    tempBoard[row][col] = '.';
                }
            }
        }
    }

    /* Checks for life on game board */
    static boolean isEmpty(char[][] oldBoard) {
        boolean noLife = true;
        for (int row = 1; row <= M; row++) {
            for (int col = 1; col <= N; col++) {
                if (oldBoard[row][col] == 'X')
                    noLife = false;
            }
        }
        return noLife;
    }

    /* Identifies if a generation is different from its successor */
    static boolean isGenerationChanged(char[][] oldBoard, char[][] newBoard) {
        boolean continueGame = false;
        for (int row = 1; row <= M; row++) {
            for (int col = 1; col <= N; col++) {
                if (oldBoard[row][col] != newBoard[row][col])
                    continueGame = true;
            }
        }
        return continueGame;
    }

    static void printBoard(char[][] board, int generationCount) {
        if (isEmpty(board))
            System.out.println("\nEveryone is dead! Game over!\n");

        System.out.println("\nGeneration #" + generationCount + ":");

        for (int row = 1; row <= M; row++) {
            for (int col = 1; col <= N; col++) {
                System.out.print(board[row][col]);
            }
            System.out.print("\n");
        }
    }

    static char[][] getNextGeneration(char[][] oldBoard, char[][] newBoard) {
        for (int row = 1; row <= M; row++) {
            for (int col = 1; col <= N; col++) {
                if ((oldBoard[row][col] == 'X' && (countNeighbors(oldBoard,
                        row, col) == 2 || countNeighbors(oldBoard, row, col) == 3))
                        || (oldBoard[row][col] == '.' && countNeighbors(
                                oldBoard, row, col) == 3)) {
                    newBoard[row][col] = 'X';
                } else {
                    newBoard[row][col] = '.';
                }
            }
        }
        return newBoard;
    }

    static char[][] copyArray(char[][] newBoard, char[][] tempBoard) {
        for (int row = 0; row <= M + 1; row++) {
            for (int col = 0; col <= N + 1; col++) {
                tempBoard[row][col] = newBoard[row][col];
            }
        }
        return tempBoard;
    }

    static int countNeighbors(char[][] board, int row, int col) {
        int count = 0;
        int tempRow = row;
        int tempCol = col;

        for (col = tempCol - 1; col <= tempCol + 1; col++) {
            if (board[row - 1][col] == 'X')
                count++;

            if (board[row + 1][col] == 'X')
                count++;
        }

        for (col = tempCol - 1; col <= tempCol + 1; col += 2) {
            if (board[row][col] == 'X')
                count++;
        }
        return count;
    }
}

Answer 1

It's great that you strive to improve your skills, even though you already got a good score.

OOP

An OOP language like Java makes it easy to encapsulate closely related data and operations in ADTs (abstract data types). An exercise like this screams for an ADT called Board.

Methods with the word "board" in the name will naturally fit into the ADT, and instead of passing the char[][] parameters around everywhere, it could be hidden neatly inside the board, applying the good principles of encapsulation and information hiding. (Recommended reading: Code Complete chapter 6)

Single responsibility principle

A method should have one responsibility, one thing to do and do it well. Most of your method do multiple things. They can be split up to smaller methods. The result will be multiple shorter methods that are easier to understand and easier to understand.

Reading from standard input

The program reads from standard input in two places, and each time it creates a new Scanner instance. It would be better to create one scanner instance in the main method, and pass that to the methods that need it.

Avoid pointless flag variables

The flag variable continueGane here is unnecessary (like flag variables often are):

    boolean continueGame = true;
    while (continueGame) {
        if (isEmpty(oldBoard)) {
            continueGame = false;
        }
        // ...

Instead of setting this variable to false, it would be simpler to just break out of the loop.

Not doing so makes the program harder to read. Here, for example, if I want to verify what else will happen until the end of this cycle, I have to read the entire loop body. By breaking out right here right now, I will know that we're definitely out of the loop, the mental burden is greatly reduced.

Another good example is in the other branch in this loop that sets the flag variable:

            } else {
                continueGame = false;
                // ... many many lines of code

                System.out.println("\n\n\nYou will now exit the program!\n\n\n");
            }

So the print statement tells me that we're exciting the loop. But on that line, far away from the statement where you set the flag, and far away from the statement of the loop, it's not obvious that we really will exit. That's an unnecessary mental burden. And making it obvious is easy:

                System.out.println("\n\n\nYou will now exit the program!\n\n\n");
                break;

And as we never change the flag variable anymore, the loop condition can be simplified to true.

Flag variables also often lead to overlooking performance issues, for example here:

static boolean isGenerationChanged(char[][] oldBoard, char[][] newBoard) {
    boolean continueGame = false;
    for (int row = 1; row <= M; row++) {
        for (int col = 1; col <= N; col++) {
            if (oldBoard[row][col] != newBoard[row][col])
                continueGame = true;
        }
    }
    return continueGame;
}

Once the flag is set to true, it will never change, but the code continues to explore the entire board, instead of returning immediately.

Magic values

Some literal values appear at multiple places here and there, for example '.' and 'X'. It would be better to put these in constants with descriptive names.

Answer 2

• Separate logic from presentation.

  Working with cells status represented directly with X and . leads to surprising performance issues. For example the most critical countNeighbours becomes a bit convoluted. Having a cell status as 1 and 0 instead, it becomes

  static int countNeighbours(char[][] board, int row, int col) {
      int count = -board[row][col];
      for (int i = -1; i < 2; ++i) {
          for (int j = -1; j < 2; ++j) {
              count += board[row + i][col + j];
          }
      }
      return count;
  }
  

• Overcrowded conditional

  The condition

  if (oldBoard[row][col] == 'X' && (countNeighbors(oldBoard,
                  row, col) == 2 || countNeighbors(oldBoard, row, col) == 3))
                  || (oldBoard[row][col] == '.' && countNeighbors(
                          oldBoard, row, col) == 3)
  

  is very hard to follow. Consider

      int neighbours = countNeighbours(oldBoard, row, col);
  
      if (neighbours == 3) {
          newBoard[row][col] = 1;
      } else if (neighbours == 2) {
          newBoard[row][col] = oldBoard[row][col];
      } else {
          newBoard[row][col] = 0;
      }
  

• Copying boards

  can be avoided. Swap the references instead:

      public static int[][] K(int[][] a, int[][] b) {
          return a;
      }
      ....
      newBoard = K(oldBoard, newBoard = oldBoard);
  

  This great technique is known as K-combinator.