Java Conway's Game Game of Life

Question

I have implemented Conway's Game of Life simulation. What can I improve? How could I structure my programme better? All suggestions would be appreciated.

World.java:

public class World {

private int WORLDSIZE;
private final int DEAD = 0;
private final int ALIVE = 1;
private int world[][];
private int worldCopy[][];

public World(int WORLDSIZE) {
    this.WORLDSIZE = WORLDSIZE;
    this.world = new int[WORLDSIZE][WORLDSIZE];
    this.worldCopy = new int[WORLDSIZE][WORLDSIZE];
}

public int getWorldSize() {
    return this.WORLDSIZE;
}

public int[][] getWorld() {
    return this.world;
}

public int getCellState(int x, int y) {

    if (world[x][y] == ALIVE)
        return ALIVE;
    else
        return DEAD;
}

public int getAlive() {
    return this.ALIVE;
}

public int getNeighborCells(int x, int y) {

    int neighbors = 0;

    for (int i = x - 1; i <= x + 1; i++) {
        for (int j = y - 1; j <= y + 1; j++) {

            if (world[i][j] == ALIVE && (x != i || y != j))
                neighbors += 1;
        }
    }

    return neighbors;
}

public void createRandomWorld() {

    for (int y = 0; y < WORLDSIZE; y++) {
        for (int x = 0; x < WORLDSIZE; x++) {

            if (Math.random() > 0.9) {
                world[x][y] = ALIVE;
            } else
                world[x][y] = DEAD;
        }
    }
}

public int[][] applyRules() {

    for (int i = 1; i < WORLDSIZE - 1; i++) {
        for (int j = 1; j < WORLDSIZE - 1; j++) {

            int neighbors = getNeighborCells(i, j);

            if (world[i][j] == ALIVE) { 

                if ((neighbors < 2) || (neighbors > 3)) {
                    worldCopy[i][j] = DEAD; 
                }

                if (neighbors == 2 || neighbors == 3) {
                    worldCopy[i][j] = ALIVE;
                }
            }

            if (world[i][j] == 0 && neighbors == 3) { 
                worldCopy[i][j] = ALIVE;
            }
        }

    }

    return worldCopy;
}

public void updateWorld() {
    world = applyRules();
}

Board.java:

import java.awt.Color;
import java.awt.Dimension;
import java.awt.Graphics; 
import javax.swing.JPanel;

public class Board extends JPanel {

private int width = 250;
private int height = 250;
private World world;
private final int CELLSIZE = 5;

public Board(World world) {
    this.world = world;
    world.createRandomWorld();      
    setPreferredSize(new Dimension(width, height));
    setBackground(Color.YELLOW);
}

@Override
public void paintComponent(Graphics g) {
    super.paintComponent(g);

    for (int i = 0; i < world.getWorldSize(); i++) {
        for (int j = 0; j < world.getWorldSize(); j++) {

            if (world.getCellState(i, j) == world.getAlive()) {
                g.setColor(Color.RED);
                g.fillRect(i * CELLSIZE, j * CELLSIZE, CELLSIZE, CELLSIZE);
            }

            else {
                g.setColor(Color.YELLOW);
                g.fillRect(i * CELLSIZE, j * CELLSIZE, CELLSIZE, CELLSIZE);
            }
        }
    }
  }
}

Simulation.java:

import java.util.Timer;
import java.util.TimerTask;

public class Simulation {

private World world;
private Board board;
private Timer timer = new Timer();
private final int period = 100;

public Simulation(Board board, World world) {
    this.board = board;
    this.world = world;
    this.simulate();
}

public void simulate() {
    timer.schedule(new TimerTask() {

        @Override
        public void run() {

            world.applyRules();
            world.updateWorld();

            board.repaint();
        }

    }, 0, period);
 }
}

Main.java:

import javax.swing.JFrame;

public class Main {

private World world;
private Board board;
private Simulation simulation;
private JFrame frame = new JFrame("GameOfLife");
private final int width = 250, height = 250;

public Main() {
    world = new World(50);
    board = new Board(world);
    simulation = new Simulation(board, world);

    frame.setSize(width, height);
    frame.add(board);
    frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);

    frame.setVisible(true);
}

public static void main(String[] args) {

    new Main();
 }
}

Thank you in advance.

Answer 1

I'm going to focus on World, because it's the simplest part and also the main logic behind the program.

The biggest change I'd make is make World immutable. Every time you make a change to World, you create a new instance of it. That simplifies thing within World and also gives you free 'history', theoretically allowing you to go back and forth between time steps. It'll also mean you don't need an apply/update.

Some things to do that:

• Make everything final: WORLDSIZE should already be final. It's not being modified anywhere, and if you want a different sized world, make a new one.
• Make your constructor private. If you want to create a world, use static builder methods to do that.

Then move away from primitives.

• Don't use int, use boolean: boolean isAlive. What if someone (like you) sets something to 3 by accident? What does that mean? Maybe you do want a cell to have more than two states at some point. Then use an enum. But you can change that later if you do need it (YAGNI).
• Don't use boolean, create your own object. If you have some kind of Cell class, then you can do things like connect neighbours, e.g. cell.getNeighbors and work with that directly. This can be initialised in the constructor. It's more hard work to start with, but it makes the other functions much more straight-forward. You can also call kill or setState(...) or whatever. Then it's self-documenting. And now you can also get rid of your 2D-arry completely, you just need a single List/array to contain a reference to all the cells!

A couple of more minor things, given the code that is there:

• Rename getNeighborCells. It doesn't get the neighbours, it gets the count.
• Similarly with createRandomWorld. It's not actually creating a world, it's initialising it. This kinda goes hand-in-hand with the immutability. This would be your builder that creates a new World object randomly initialised.
• You might consider taking the logic out of the applyRules and passing it in as a "strategy". This would be easy with lambdas (with later versions of Java), and you could also provide a series of static, standard strategies, even a default by overloading the method.
• You have if (world[i][j] == ALIVE) and then if (world[i][j] == 0): this is an example of the problem caused by ints. How do you know 0 is DEAD? The compiler doesn't, so it can't check if that's actually what you mean.
• Simplify those ifs in the apply method: the second one can be an else, for instance (it's actually not needed at all because you've already checked it's ALIVE).

As far as I can see, the whole logic can be simplified to:

if (numberOfNeighbors < 2 || numberOfNeighbors > 3) {
  cell.kill();
} else if (numberOfNeighbors == 3) {
  cell.resurrect();
}

Another advantage of moving to your own custom objects which you can interact with directly is you can take advantage of Java streams (although seeing your Timer, I'm assuming you're using an ooolder version):

public World applyStrategy(Function<Cell, CellState> strategy) {
  return new World(allCells.stream()
      .map(cell -> Cell.createCell(cell, strategy.apply(cell)))
      .toArray(String[]::new))
}

// In main...
World latestIteration = lastIteration.applyStrategy(cell -> {
  int numberOfNeighbors = cell.getNeighborCount();
  if (numberOfNeighbors < 2 || numberOfNeighbors > 3) {
    return CellState.DEAD;
  } else if (numberOfNeighbors == 3) {
    return CellState.ALIVE;
  }
  return cell.getState();
}

Now that your World class is so simple (just an array of cell references), you can pass the new array/list of cells to a private constructor. And you would need a constructor in your Cell class for copying a given Cell and applying the given state (or, as given here, a static builder method).

Edit: Although you could change it to a List, given the size never changes, you can happily leave it as an array.

Answer 2

Separate construction and action

Avoid performing any actions in constructors. The purpose of constructors is to create objects that are ready to use. The construction of an object should not trigger actions.

For example:

• The constructor of Main triggers everything in the program. This is not expected from a constructor. Move this code from the constructor to the static main method. That is an appropriate place to drive a program.

• The constructor of Simulation starts the simulation. Remove the line starts the simulation, let the owner of the object make that call.

• The constructor of Board takes a World as parameter, and calls createRandomWorld on it which will mutate the world, reinitializing its content. It is unexpected from a constructor to mutate its parameter.

Double computation

Consider this snippet in Simulation:

world.applyRules();
world.updateWorld();

Did you notice that calling world.applyRules() has no effect? It computes the new state of the world and returns it, which is not stored anywhere. In fact it's unnecessary here, because world.updateWorld() makes that call, to replace the state of the world with the new state. And I don't see a need to separate these two methods.

Also note that in a class named "World", including the term "world" in method names is usually redundant. In this example, you could drop it and world.update() will be naturally readable.

Scope

Many variables are defined as class fields, when they can be local variables. Always try to limit variables to the minimum scope where they are needed. This reduces the chance of accidental uses and modifications.

For example worldCopy is only used in one method that computes the new state. It's a low-level implementation detail of that method, and so it should only be visible there.

I suggest to review all other variables too and ask yourself if it really needs to be visible at the scope, or if it can be moved into a smaller scope.

Separation of concerns and naming

There are some distinct concepts that are easy to grasp in this program:

• A board that represents the state of the world
• Displaying the board
• Driving the program from state to state

How does the program map to these concepts?

• The World class manages the state of the board
• The Board class displays the board
• The Simulation drives the program, started from Main

I think World and Board are not well separated. From their names I cannot tell their purpose, I need to look at the implementation, and it surprises me, because World manages a board, while Board manages display of a board.

In short, I would rename World to Board, and Board to BoardPanel.

With this renaming, it also becomes clear that BoardPanel should not modify the state of a Board, so calling board.createRandomWorld is inappropriate. BoardPanel should be only an observer of the state of the board, it should not modify it.