4
\$\begingroup\$

This is a simple implementation of Conway's Game of Life in Java. The algorithm I use is inefficient however. Every Cell's neighbors (alive or dead) are checked on every iteration.

import java.util.Scanner;

public class GameOfLife {

    public final static int BOARD_HEIGHT = 20;
    public static Cell[][] board = new Cell[BOARD_HEIGHT][BOARD_HEIGHT];
    public static Cell[][] lastIteration = new Cell[BOARD_HEIGHT][BOARD_HEIGHT];
    public static final int TIME_BETWEEN_ITERATIONS_MS = 250;
    public static final char DEAD_CELL_SYMBOL = '□';
    public static final char ALIVE_CELL_SYMBOL = '■';

    private enum Cell {
        DEAD, ALIVE
    }

    public static void printBoard(Cell[][] board) {
        for (int i = 0; i < BOARD_HEIGHT; i++) {
            for (int j = 0; j < BOARD_HEIGHT; j++) {
                switch (board[i][j]) {
                case DEAD:
                    System.out.print(DEAD_CELL_SYMBOL);
                    break;
                case ALIVE:
                    System.out.print(ALIVE_CELL_SYMBOL);
                    break;
                }
                System.out.print(' ');
            }
            System.out.println();
        }
        System.out.println();
    }

    public static Cell changeCell(int x, int y, Cell[][] boardArg) {
        int liveNeighbours = 0;
        for (int i = -1; i < 2; i++) {
            for (int j = -1; j < 2; j++) {
                if (x + i >= 0 && y + j >= 0 && x + i < BOARD_HEIGHT && y + j < BOARD_HEIGHT) {
                    if (boardArg[x + i][y + j] == Cell.ALIVE && !(i == 0 && j == 0)) {
                        liveNeighbours++;
                    }
                }
            }
        }

        if (liveNeighbours < 2 && boardArg[x][y] == Cell.ALIVE)
            return Cell.DEAD;
        else if ((liveNeighbours == 2 || liveNeighbours == 3) && boardArg[x][y] == Cell.ALIVE)
            return Cell.ALIVE;
        else if (liveNeighbours > 3 && boardArg[x][y] == Cell.ALIVE)
            return Cell.DEAD;
        else if (boardArg[x][y] == Cell.DEAD && liveNeighbours == 3)
            return Cell.ALIVE;
        else
            return Cell.DEAD;
    }

    public static void main(String[] args) throws InterruptedException {

        for (int i = 0; i < BOARD_HEIGHT; i++) {
            for (int j = 0; j < BOARD_HEIGHT; j++) {
                board[i][j] = Cell.DEAD;
            }
        }

        Scanner reader = new Scanner(System.in);
        int numberOfLiveCells = reader.nextInt();

        for (int i = 0; i < numberOfLiveCells; i++) {
            int x = reader.nextInt();
            int y = reader.nextInt();
            board[x][y] = Cell.ALIVE;
        }

        reader.close();
        printBoard(board);

        while (true) {
            for (int i = 0; i < BOARD_HEIGHT; i++) {
                for (int j = 0; j < BOARD_HEIGHT; j++) {
                    lastIteration[i][j] = board[i][j];
                }
            }

            for (int i = 0; i < BOARD_HEIGHT; i++) {
                for (int j = 0; j < BOARD_HEIGHT; j++) {
                    board[i][j] = changeCell(i, j, lastIteration);
                }
            }
            printBoard(board);
            Thread.sleep(TIME_BETWEEN_ITERATIONS_MS);
        }
    }
}
\$\endgroup\$
4
\$\begingroup\$

pro

  • you respect Java naming conventions
  • you separate different parts in methods

contra

use of static methods

Java is an object oriented language so the use of objects is encouraged.

in your code you'd need one more method to move the code from main to and then you would call that method on a new instance of your class:

public static void main(String[] args) throws InterruptedException {
   new GameOfLife().run();
}

private void run() throws InterruptedException {
    for (int i = 0; i < BOARD_HEIGHT; i++) {
        for (int j = 0; j < BOARD_HEIGHT; j++) {
            board[i][j] = Cell.DEAD;
        }
    }
 //...

then you should remove the static key word from all methods except main. You could (and should) also remove the static key word from most of your variables, except the constants.

enums used in switch

In Java enums are full featured class. That means it is possible benefit from polymorphism.

In your printBoard() method you have this:

           switch (board[i][j]) {
            case DEAD:
                System.out.print(DEAD_CELL_SYMBOL);
                break;
            case ALIVE:
                System.out.print(ALIVE_CELL_SYMBOL);
                break;

when we change the enum Cell to this:

private enum Cell {
    DEAD(DEAD_CELL_SYMBOL), ALIVE(ALIVE_CELL_SYMBOL);
    private final String symbol;
    Cell(String symbol){
      this.symbol = symbol;
    }
    String getSymbol(){
      return symbol;
    }
}

the code in printBoard would change to this:

System.out.print(board[i][j].getSymbol()); // that replaces the whole switch!

something similar is possible for the calculation of the next state.

you have a rather complicated list of ifstatements:

  if (liveNeighbours < 2 && boardArg[x][y] == Cell.ALIVE)
        return Cell.DEAD;
    else if ((liveNeighbours == 2 || liveNeighbours == 3) && boardArg[x][y] == Cell.ALIVE)
        return Cell.ALIVE;
    else if (liveNeighbours > 3 && boardArg[x][y] == Cell.ALIVE)
        return Cell.DEAD;
    else if (boardArg[x][y] == Cell.DEAD && liveNeighbours == 3)
        return Cell.ALIVE;
    else
        return Cell.DEAD;

if you have a closer look at it this splits up in two parts:

  • the cell is currently dead
  • the cell is currently alive

this is perfectly eligible to be moved into the Cell enum since this removes one of the if cases.

the enum would change to this:

private enum Cell {
    DEAD(DEAD_CELL_SYMBOL){
      @Override 
      Cell getNext(int liveNeigbours){
        return liveNeighbours == 3? ALIFE:DEAD; 
      }
    }, 
    ALIVE(ALIVE_CELL_SYMBOL){
      @Override 
      Cell getNext(int liveNeigbours){
        return liveNeighbours == 2|| liveNeighbours == 3? ALIFE:DEAD; 
      }
    };
    private final String symbol;
    Cell(String symbol){
      this.symbol = symbol;
    }
    abstract Cell getNext(int liveNeigbours); 
    String getSymbol(){
      return symbol;
    }
}

the code in changeCell would change to this:

 return boardArg[x][y].getNext(liveNeigbours); // replaces all the `if`s
| improve this answer | |
\$\endgroup\$
5
\$\begingroup\$

Unicode - nice

I see you directly use Unicode. I like that.

Incorrect solution

Your program is actually NOT an implementation of Game of Life, because your universe is finite. The universe size is limited to 20×20, which is very tiny. A good implementation of Game of Life would feature a Universe of size 2³²×2³² which is still finite but feels infinite for practical purposes. Think of a different data structure. Instead of managing the entire space-time continuum of the Game of Life universe, manage only the matter. Stop caring about dead cells, care about life cells only.

Test

Your program lacks testing, which I can see from the next point.

Do not use static for mutable fields

You have static variables which are not final, which means that your program has only one shared mutable state. Holding mutable state is one of the things for which OO languages like Java provide objects.

Use the power of enum

enums aren't mere enumarations of constants. In other languages they are. In Java, enums are special classes, which describe a predefined, finite, immutable set of objects (as opposed to a normal class which describes a set of objects which is not predefined, infinite and mutable). In this case you could give enum Cell a toString() method which would return the cell symbol. Then you would not need a switch in printBoard().

In general, switch statements (and if statements that are switch statements in disguise) indicate a lack of OO-design. Often, they can be replaced with polymorphism.

You could even create an abstract method in Cell which generates the next cell based on the number of life neighbors and implemented it differently for alive and dead cells.

Separate the construction of output from the actual output

Method printBoard() is doing two things and thus difficult to test (and a bit inefficient):

  • It constructs the output.
  • It prints the output.

Consider separating these two things. Create one method which constructs the output, maybe using a StringBuilder. And the other method would only print the output. It will give you two benefits:

  • The program will be faster (which probably is no concern here).
  • The program will be easier to test.

Use the same format for input as for output

Currently, you use coordinates for input and a Unicode-Art representation of the universe for output. That means the program's output cannot be fed into the program as input again. Imagine how a test could look like if the output can be fed back to the program as input.

JUnit Example

@Test
public void testBlinker() {
    final Universe blinkerFrame1 = Universe.parse(".*\n.*\n.*");
    final Universe blinkerFrame2 = Universe.parse("...\n***");
    assertEquals(blinkerFrame2, blinkerFrame1.iterate());
    assertEquals(blinkerFrame1, blinkerFrame2.iterate());
}

Gherkin / Cucumber Example

Given the following Universe:
  """
  .*.
  .*.
  .*.
  """
When iterating it once,
Then it MUST be equal to this:
  """
  ...
  ***
  ...
  """

Long main() method

The main() method is quite long. You could split it into a method that reads the Universe from the input, and a method which loops for the generations. The method which loops for the generations should then also be split.

Use braces as an opportunity to extract.

Links

Here's what I consider beautiful implementations of Game of Life:

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

Don't repeat work

        for (int i = -1; i < 2; i++) {
            for (int j = -1; j < 2; j++) {
                if (x + i >= 0 && y + j >= 0 && x + i < BOARD_HEIGHT && y + j < BOARD_HEIGHT) {
                    if (boardArg[x + i][y + j] == Cell.ALIVE && !(i == 0 && j == 0)) {
                        liveNeighbours++;
                    }
                }
            }
        }

You can simplify the math here. Consider

        int rightEdge = Math.min(x + 1, boardArg.length - 1);
        int bottomEdge = Math.min(y + 1, boardArg[0].length - 1);
        int topEdge = Math.max(y - 1, 0)
        for (int i = Math.max(x - 1, 0); i <= rightEdge; i++) {
            for (int j = topEdge; j <= bottomEdge; j++) {
                if (boardArg[i][j] == Cell.ALIVE && (i != x && j != y)) {
                    liveNeighbours++;
                    if (liveNeighbours > 3) {
                        return Cell.DEAD;
                    }
                }
            }
        }

Rather than generating nine neighbors and testing which are valid, this only generates neighbors that are on the board. The only invalid one is the cell itself.

We do the math that is invariant to the loops before the loops. Note that the declaration of i takes place before the loop iterates. The initial declaration of j is invariant to i but occurs inside the i loop. So we precalculate its initial value as topEdge. We do each operation only once.

If we find a fourth live neighbor, we can go ahead and quit. We don't have to continue checking neighbors.

Rather than rely on the parallel logic of the BOARD_HEIGHT constant, I use the actual size of the array. Note that I do assume that all the rows of the array are the same length.

I may have top and bottom the opposite of how your display works. If so, please switch them.

Keep it simple

        if (liveNeighbours < 2 && boardArg[x][y] == Cell.ALIVE)
            return Cell.DEAD;
        else if ((liveNeighbours == 2 || liveNeighbours == 3) && boardArg[x][y] == Cell.ALIVE)
            return Cell.ALIVE;
        else if (liveNeighbours > 3 && boardArg[x][y] == Cell.ALIVE)
            return Cell.DEAD;
        else if (boardArg[x][y] == Cell.DEAD && liveNeighbours == 3)
            return Cell.ALIVE;
        else
            return Cell.DEAD;

This is more complicated than it needs to be.

        if (liveNeighbors == 3 || (liveNeighbors == 2 && boardArg[x][y] == Cell.ALIVE)) {
            return Cell.ALIVE;
        } else {
            return Cell.DEAD;
        }

If there are three live neighbors, the cell is always alive. If there are two, then the cell is only alive if it already was. In all other cases, the cell is dead. The rules are that simple.

They make it sound more complicated by talking about over and under population and reproduction, but the rules boil down to two live cases.

We had previously returned if there more than three live neighbors, but this code will handle that case as well. That's just an optimization to save extra checks. You may want to test if it actually helps in practice or not.

I prefer to always use the block form of control structures. It helps avoid certain kinds of editing mistakes.

Don't copy twice

            for (int i = 0; i < BOARD_HEIGHT; i++) {
                for (int j = 0; j < BOARD_HEIGHT; j++) {
                    lastIteration[i][j] = board[i][j];
                }
            }

            for (int i = 0; i < BOARD_HEIGHT; i++) {
                for (int j = 0; j < BOARD_HEIGHT; j++) {
                    board[i][j] = changeCell(i, j, lastIteration);
                }
            }
            printBoard(board);
            Thread.sleep(TIME_BETWEEN_ITERATIONS_MS);

You could instead do

            for (int i = 0; i < lastIteration.length; i++) {
                for (int j = 0; j < lastIteration[i].length; j++) {
                    lastIteration[i][j] = changeCell(i, j, board);
                }
            }
            printBoard(lastIteration);
            Thread.sleep(TIME_BETWEEN_ITERATIONS_MS);

            for (int i = 0; i < board.length; i++) {
                for (int j = 0; j < board[i].length; j++) {
                    board[i][j] = changeCell(i, j, lastIteration);
                }
            }
            printBoard(board);
            Thread.sleep(TIME_BETWEEN_ITERATIONS_MS);

This switches between the two boards rather than copying one to the other.

Note that the names should probably change to match. E.g. odd and even. Or make it a three dimensional array.

Consider abstracting this into a method to reduce the duplicated code. E.g.

            processTurn(odd, even);
            processTurn(even, odd);

Where the method would contain the seven lines that are repeated.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.