Game of Life - Basic Java GUI Implementation

I've been working on a basic implementation of Conway's Game of Life over the last day, and I've just finished a 'playable' version.

If anyone has the time, would you mind making any suggestions on code improvements that I can make?

Thanks!

GameOfLife.java

```
package robbie.gameoflife.main;

import java.awt.Dimension;
import java.util.Timer;
import java.util.TimerTask;

import javax.swing.JFrame;

import robbie.gameoflife.cells.Cell;
import robbie.gameoflife.cells.CellManager;

public class GameOfLife {

private static final String TITLE = "Game of Life";

public static final int CELL_SIZE = 10, CELL_AMOUNT = 50;
public static final int WIDTH = CELL_SIZE * CELL_AMOUNT, HEIGHT = WIDTH;

public static final Dimension DIMENSIONS = new Dimension(WIDTH, HEIGHT);

private static final boolean RUNNING = true;

public static final Cell[][] CELLS = new Cell[CELL_AMOUNT][CELL_AMOUNT];

public static void main(String[] args) {
JFrame frame = new JFrame(TITLE);
frame.setSize(DIMENSIONS);
frame.setLocationRelativeTo(null);
frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
frame.setVisible(true);

GameCanvas canvas = new GameCanvas();
frame.add(canvas);
canvas.createBufferStrategy(3);

loadCells();

CELLS[10][10].setAlive(true);
CELLS[10][11].setAlive(true);
CELLS[10][12].setAlive(true);
CELLS[11][10].setAlive(true);
CELLS[12][10].setAlive(true);
CELLS[12][11].setAlive(true);
CELLS[12][12].setAlive(true);
CELLS[11][12].setAlive(true);

startGameLoop(canvas);
}

private static void loadCells() {
for (int x = 0; x < CELLS.length; x++) {
for (int y = 0; y < CELLS[x].length; y++) {
if (CELLS[x][y] == null)
CELLS[x][y] = new Cell(x, y);
}
}
}

private static void startGameLoop(GameCanvas canvas) {
Timer timer

Prefer interfaces to implementations

public static ArrayList getNeighbourCells(Cell cell) {
        ArrayList neighbourCells = new ArrayList();

and

ArrayList deadCells = new ArrayList();
        ArrayList aliveCells = new ArrayList();

You never use ArrayList specific functionality though, just List functionality (add and implicitly the iterator).

If you make these

public static List getNeighbourCells(Cell cell) {
        List neighbourCells = new ArrayList<>();

and

List deadCells = new ArrayList<>();
        List aliveCells = new ArrayList<>();

Then you can change implementations by simply changing the right side.

Note that the <> syntax only works in newer versions of Java.

Prefer doing it right to checking if right

int cellX = cell.getX();
        int cellY = cell.getY();

for (int x = cellX - 1; x = GameOfLife.CELL_AMOUNT || y = GameOfLife.CELL_AMOUNT)
                    continue;

You can simplify this with Math.min and Math.max.

int startX = cell.getX() - 1;
        int startY = cell.getY() - 1;

        int maxX = Math.min(startX + 2, GameOfLife.CELL_AMOUNT);
        int maxY = Math.min(startY + 2, GameOfLife.CELL_AMOUNT);

        for (int x = Math.max(startX, 0); x <= maxX; x++) {
            for (int y = Math.max(startY, 0); y <= maxY; y++) {

Now we don't check nine times (including once with the original values) if the x, y pair is in bounds (four checks per pair). We only check each boundary once (four boundaries).

The continue is unnecessary here

Cell neighbourCell = GameOfLife.CELLS[x][y];
                if (neighbourCell.equals(cell))
                    continue;

if (neighbourCell.isAlive())
                    neighbourCells.add(neighbourCell);

While the early bailout is often helpful, I'm not sure that I'd do it here. Consider

Cell neighbourCell = GameOfLife.CELLS[x][y];
                if (!neighbourCell.equals(cell) && neighbourCell.isAlive()) {
                    neighbourCells.add(neighbourCell);
                }

Since that's now the entire body of the loop, we don't need to continue.

I also added { and } because I prefer to always use the block form. There's a set of problems that can arise with inattentive editing of the statement form that don't bother the block form. I just find it easier to always use it. The maintenance cost is cheaper in my opinion.

Don't Repeat Yourself

if (neighbourCount  3)
                    deadCells.add(cell);
                else if (cell.isAlive() && (neighbourCount == 2 || neighbourCount == 3))
                    aliveCells.add(cell);
                else if (!cell.isAlive() && neighbourCount == 3)
                    aliveCells.add(cell);

This is the same thing as

if (neighbourCount  3) {
                    deadCells.add(cell);
                } else if (cell.isAlive()) {
                    aliveCells.add(cell);
                } else if (neighbourCount == 3) {
                    aliveCells.add(cell);
                }

The else statement lets you know that the expression in the if is not true. You don't have to check it again. So we know there are 2 or 3 neighbors in the first and we know that the cell is alive in the second (and we still know that there are 2 or 3 neighbors, although that doesn't help since 2 and 3 neighbor cells have different behavior when not alive).

WE can continue to simplify:

if (neighbourCount  3) {
                    deadCells.add(cell);
                } else if (cell.isAlive() || neighbourCount == 3) {
                    aliveCells.add(cell);
                }

Given that what you are doing is setting the alive status, we can do even better in terms of efficiency.

if (neighbourCount  3) {
                    deadCells.add(cell);
                } else if (!cell.isAlive() && neighbourCount == 3) {
                    aliveCells.add(cell);
                }

Now the second if only puts the cell in the list if it is not alive but should be next round. We can do the same thing to the first if.

if (cell.isAlive() && (neighbourCount  3)) {
                    deadCells.add(cell);
                } else if (!cell.isAlive() && neighbourCount == 3) {
                    aliveCells.add(cell);
                }

But now we call cell.isAlive() twice, once in each branch. This makes the else redundant. We could just as well write

if (cell.isAlive() && (neighbourCount  3)) {
                    deadCells.add(cell);
                }

                if (!cell.isAlive() && neighbourCount == 3) {
                    aliveCells.add(cell);
                }

or

```
if (cell.isAlive() && neighbourCount != 2 && neighbourCount != 3) {
deadCells.add(cell);
}

if (!cell.isAlive() && neighbourCount == 3) {
aliveCells.add(cell);

StackExchange Code Review Q#139125, answer score: 2