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 = new Timer();
if (RUNNING) {
timer.schedule(new TimerTask() {
@Override
public void run() {
CellManager.updateCells();
canvas.repaint();
}
}, 1000, 1000);
}
}
}
GameCanvas.java
package robbie.gameoflife.main;
import java.awt.Canvas;
import java.awt.Color;
import java.awt.Graphics;
import java.util.Random;
import robbie.gameoflife.cells.Cell;
public class GameCanvas extends Canvas {
public GameCanvas() {
super();
this.setSize(GameOfLife.WIDTH, GameOfLife.HEIGHT);
this.setBackground(Color.WHITE);
this.setFocusable(true);
this.setVisible(true);
}
public void paint(Graphics g) {
Graphics graphics = this.getGraphics();
graphics.clearRect(0, 0, GameOfLife.WIDTH, GameOfLife.HEIGHT);
Random random = new Random();
graphics.setColor(new Color(random.nextFloat(), random.nextFloat(), random.nextFloat()));
for (Cell[] cells : GameOfLife.CELLS) {
for (Cell cell : cells) {
if (cell.isAlive()) {
int size = GameOfLife.CELL_SIZE;
int x = cell.getX() * size;
int y = cell.getY() * size;
graphics.fillRect(x, y, size, size);
}
}
}
}
}
Cell.java
package robbie.gameoflife.cells;
public class Cell {
private int x, y;
private boolean alive = false;
public Cell(int x, int y) {
this.x = x;
this.y = y;
}
public int getX() {
return x;
}
public int getY() {
return y;
}
public boolean isAlive() {
return alive;
}
public void setAlive(boolean alive) {
this.alive = alive;
}
public void setX(int x) {
this.x = x;
}
public void setY(int y) {
this.y = y;
}
public void toggleAlive() {
this.alive = !this.alive;
}
}
CellManager.java
package robbie.gameoflife.cells;
import java.util.ArrayList;
import robbie.gameoflife.main.GameOfLife;
public class CellManager {
public static ArrayList<Cell> getNeighbourCells(Cell cell) {
ArrayList<Cell> neighbourCells = new ArrayList<Cell>();
int cellX = cell.getX();
int cellY = cell.getY();
for (int x = cellX - 1; x <= cellX + 1; x++) {
for (int y = cellY - 1; y <= cellY + 1; y++) {
if (x < 0 || x >= GameOfLife.CELL_AMOUNT || y < 0 || y >= GameOfLife.CELL_AMOUNT)
continue;
Cell neighbourCell = GameOfLife.CELLS[x][y];
if (neighbourCell.equals(cell))
continue;
if (neighbourCell.isAlive())
neighbourCells.add(neighbourCell);
}
}
return neighbourCells;
}
public static int getNeighbourCount(Cell cell) {
return getNeighbourCells(cell).size();
}
public static void updateCells() {
ArrayList<Cell> deadCells = new ArrayList<Cell>();
ArrayList<Cell> aliveCells = new ArrayList<Cell>();
for (Cell[] cells : GameOfLife.CELLS) {
for (Cell cell : cells) {
int neighbourCount = CellManager.getNeighbourCount(cell);
if (neighbourCount < 2 || 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);
}
}
for (Cell cell : aliveCells)
cell.setAlive(true);
for (Cell cell : deadCells)
cell.setAlive(false);
}
}
1 Answer 1
Prefer interfaces to implementations
public static ArrayList<Cell> getNeighbourCells(Cell cell) { ArrayList<Cell> neighbourCells = new ArrayList<Cell>();
and
ArrayList<Cell> deadCells = new ArrayList<Cell>(); ArrayList<Cell> aliveCells = new ArrayList<Cell>();
You never use ArrayList specific functionality though, just List functionality (add and implicitly the iterator).
If you make these
public static List<Cell> getNeighbourCells(Cell cell) {
List<Cell> neighbourCells = new ArrayList<>();
and
List<Cell> deadCells = new ArrayList<>();
List<Cell> 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 <= cellX + 1; x++) {
for (int y = cellY - 1; y <= cellY + 1; y++) {
if (x < 0 || x >= GameOfLife.CELL_AMOUNT || y < 0 || 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 < 2 || 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 < 2 || 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 < 2 || 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 < 2 || 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 < 2 || 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 < 2 || 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);
}
This takes the negation of the or expression so that we can do without the extra parentheses.
Or even
if (cell.isAlive()) {
if (neighbourCount != 2 && neighbourCount != 3) {
deadCells.add(cell);
}
} else if (neighbourCount == 3) {
aliveCells.add(cell);
}
Although the complexity of nesting might make it less readable. I would consider any of these last three to be reasonable.
Note that even in your original, there was a case, !cell.isAlive() && neighbourCount == 2 that doesn't get set either way. This just makes that clearer.
Naming
Also consider changing deadCells and aliveCells to something that makes it clearer that the status is changing, e.g. dyingCells and birthingCells. Otherwise, you could just write
if (neighbourCount == 3 || (neighbourCount = 2 && cell.isAlive())) {
aliveCells.add(cell);
} else {
deadCells.add(cell);
}
This sets every cell's status explicitly.
A bit wasteful though, as the majority of cells will not be alive and not be changing status.