Game Of Life rewritten into two classes, PetriDish and Cell

Follow-up to: Game Of Life implemented with for-loops and a boolean-array

As proposed by palacsint I went on and rewrote my implementation of the Game Of Life and extracted everything into two classes:

-
PetriDish

• The parent which holds the array of cells

• Sets the array up and initializes each cell

• Starts the evolution

-
Cell

• Stores it's current value

• Holds references to it's neighbors

The main cell array is still one-dimensional because I find it convenient to access all cells easily with a simple foreach loop. The outermost cells are a dead border and never change there state, mostly to ease handling of the array.

It works like this:

• Initiate the PetriDish

• Set the array

• Add cells into the array

• Initialize all cells

• The cell fetches it's neighbors from the parent PetriDish, except if it is a border-cell, then it does nothing.

• The PetriDish kicks off the evolution

• The cell checks it's neighbors via two static methods, countAliveNeighbors and checkSurvival

• It stores that value in a second variable

• The PetriDish ends the evolution

• The cell overrides it's old value with the new one from the above cycle

This has a few ugly quirks:

• Handling of the border-cells feels a little bit odd, but while I think about it I could remove them completely

• It's necessary to save the new value of the cell in another variable, there for makes the evolution a two-step process

Okay, enough talk, here's the code:

```
public class PetriDish {

private Cell[] cells;
private int width;
private int height;
private long generation;
private long duration;

public Cell[] getCells() {
return cells;
}

public Cell getCell(int x, int y) {
return cells[y * width + x];
}

public int getHeight() {
return height;
}

public int getWidth() {
return width;
}

public long getGeneration() {
return generation;
}

public long getDuration() {

-
Getter-setter methods in PetriDish and Cell should be declared after the constructors. (Check Java Coding Conventions, 3.1.3 Class and Interface Declarations.) I prefer placing them at the end of the classes (after every other methods).

-
I'd check the input values:

import static com.google.common.base.Preconditions.*;
...

public PetriDish(final int width, final int height) {
    checkArgument(width > 3, "width has to be at least 3");
    checkArgument(height > 3, "height has to be at least 3");
    ...
}

Of course a simple if and a throw new IllegalArgumentException also does it if you don't want to include guava-libraries.

-
Passing this to another method/class in the constructor usually isn't a good idea:

cells[y * width + x] = new Cell(this, x, y, x == 0 || y == 0 
    || x == width - 1 || y == height - 1);

If the constructor later throws an exception the created Cell objects has a reference to an incompletely constructed PetriDish object. (Maybe Cell stores the reference in a static field, passes it to an event listener or a singleton class etc.) I'm not sure, but probably I'd create an init() method for this logic.

-
Reading the following constructor call isn't the easiest task:

new Cell(this, x, y, x == 0 || y == 0 || x == width - 1 || y == height - 1);

I'd create a local variable for the last parameter:

final boolean isBorderCell = x == 0 || y == 0 || x == width - 1 
    || y == height - 1;
cells[y * width + x] = new Cell(this, x, y, isBorderCell);

An isBorderCell method would be the best:

private boolean isBorderCell(final int width, final int height, final int x, 
        final int y) {
    if (x == 0) {
        return true;
    }
    if (y == 0) {
        return true;
    }
    if (x == width - 1) {
        return true;
    }
    if (y == height - 1) {
        return true;
    }
    return false;
}

I think it's easier to read than a long boolean expression.

-
I'd rename Cell.prepare() to init(). It's more common.

-
The prepare method could be in the PetriDish class and could call a Cell.setNeighbors(). If you move it Cell doesn't have to have PetriDish field and constructor parameter at all.

-
countAliveNeighbors should be non-static and should use the neighbors field directly:

private int countAliveNeighbors() {
    ...
}

The same is true for the checkSurvival method. Actually, I'd rename it to calcNextState:

private boolean calcNextState() {
    final int neighborsAlive = countAliveNeighbors();
    switch (neighborsAlive) {
        ...
    }
}

-
Use getters instead of direct field access in the countAliveNeighbors method:

if (neighbor.getValue()) {
    neighborsAlive++;
}

(What's the deal with Java's public fields?)

-
Instead of the boolean value flag I'd use an enum with ALIVE and DEAD values. It would be more meaningful (without true and false as magic "numbers").

-
The isBorderCell flag in Cell does not smell good. Create a Cell interface:

public interface Cell {
    void prepare();
    void startEvolution();
    void finishEvolution();
    boolean getValue();
    void setValue(final boolean value);
}

and rename the current Cell to NormalCell and create an empty BorderCell class:

public class BorderCell implements Cell {

    @Override
    public void prepare() {
    }

    @Override
    public void startEvolution() {
    }

    @Override
    public void finishEvolution() {
    }

    @Override
    public boolean getValue() {
        return false;
    }

    @Override
    public void setValue(boolean value) {
        throw new UnsupportedOperationException();
    }
}

A factory method is useful in the PetriDish:

private Cell createCell(int x, int y) {
    if (isBorderCell(width, height, x, y)) {
        return new BorderCell();
    } else  {
        return new NormalCell(this, x, y);
    }
}

After that remove the borderCell flag and its usages from the NormalCell class. (Check Replacing the Conditional Logic on Price Code with Polymorphism in Refactoring: Improving the Design of Existing Code by Martin Fowler)

-
If you would like to get rid of the nextValue field in Cell try modifying the doEvolution method. It could return a new PetriDish with brand new Cells which contain the state of the next generation. It would help storing the full history (for example in a List), but I think the current design with the nextValue field also fine (if you don't need the full history of course).

StackExchange Code Review Q#6664, answer score: 4