Game of life GUI simulation using java.swing

Question

I am fairly new to GUI programming and I am interested in improving my code from every aspect. Performance, security, readability, conciseness, and the look-and-feel aspects are all important to me.

Here is the non-GUI part of the code:

import java.util.Random;

public class Universe
{
    private int generation;
    private int alive;
    private boolean[][] currentGeneration;
    private boolean[][] nextGeneration;
    private Random random;

public Universe(int height, int width, int seed, String pattern)
{
   this.currentGeneration = new boolean[height][width];
   if (pattern.equalsIgnoreCase("Random"))
   {
       random = new Random(seed);
       for (int i = 0; i < height; i++) 
       {
           for (int j = 0; j < width; j++) 
           {
               currentGeneration[i][j] = random.nextBoolean();
           }
       }
   }
   else if (pattern.equalsIgnoreCase("glider"))
   {
       getGlider(currentGeneration);
   }
   //to add more cases here
    nextGeneration = generateNextGeneration(currentGeneration);
    generation = 1;
    alive = calculateAlive(currentGeneration);
}

//Getters and instance methods

int getGeneration()
{
    return generation;
}
int getAlive()
{
    return alive;
}
boolean[][] getCurrentGeneration()
{
    return currentGeneration;
}
boolean[][] getNextGeneration()
{
    return nextGeneration;
}
void moveToNextState()
{
    boolean[][] temp = generateNextGeneration(nextGeneration);
    currentGeneration = nextGeneration;
    nextGeneration = temp;
    alive = calculateAlive(currentGeneration);
    generation++;
}
void reset(int h, int w, int seed)
{
    this.currentGeneration = new boolean[h][w];
    random = new Random(seed);
    for (int i = 0; i < h; i++)
    {
        for (int j = 0; j < w; j++)
        {
            this.currentGeneration[i][j] = random.nextBoolean();
        }
    }
    nextGeneration = generateNextGeneration(currentGeneration);
    generation = 1;
    alive = calculateAlive(currentGeneration);
}

//Utility methods

static int calculateNeighbours(boolean[][] grid, int row, int column)
{
    int neighbours = 0, r, c;
    int N = grid.length;
    int M = grid[0].length;

    for (int p = -1; p <= 1; p++)
    {
        for (int m = -1; m <= 1; m++)
        {
            r = row + p;
            c = column + m;

            if (r < 0)
                r = N - 1;
            if (r > N - 1)
                r = 0;
            if (c < 0)
                c = M - 1;
            if (c > M - 1)
                c = 0;

            if (grid[r][c] && (p != 0 || m != 0))
                neighbours++;
        }
    }
    return neighbours;
}

static int calculateAlive(boolean[][] grid)
{
    int alive = 0;
    for (int i = 0; i < grid.length; i++)
    {
        for (int j = 0; j < grid[0].length; j++)
        {
            if (grid[i][j])
                alive++;
        }
    }
    return alive;
}

static boolean[][] generateNextGeneration(boolean[][] currentGeneration)
{
    int N = currentGeneration.length;
    int M = currentGeneration[0].length;
    boolean[][] nextGeneration = new boolean[N][M];
    int neighbours;
    for (int i = 0; i < N; i++)
    {
        for (int j = 0; j < M; j++)
        {
            neighbours = calculateNeighbours(currentGeneration, i, j);

            if (neighbours == 3 || (currentGeneration[i][j] && neighbours == 2))
                nextGeneration[i][j] = true;
            else
                nextGeneration[i][j] = false;
        }
    }
    return nextGeneration;
}

static boolean[][] generateNthGeneration(boolean[][] currentGeneration, int X)
{
    if (X == 0)
        return currentGeneration;
    else
        return generateNthGeneration(generateNextGeneration(currentGeneration), X - 1);
}
static void printGeneration(boolean[][] generation)
{
    for (int i = 0; i < generation.length; i++)
    {
        for (int j = 0; j < generation[0].length; j++)
            System.out.print(generation[i][j]? "O" : " ");
        System.out.println();
    }

}

static void getGlider(boolean currentGeneration[][])
{
    for(int i = 0; i < 60; i++)
    {
        for (int j =0; j < 90; j++)
            currentGeneration[i][j] = false;
    }
    currentGeneration[1][3] = true;
    currentGeneration[2][3] = true;
    currentGeneration[3][3] = true;
    currentGeneration[2][1] = true;
    currentGeneration[3][2] = true;
}

}

And here is the GUI part of the code:

import javax.swing.*;
import java.awt.*;
import java.awt.event.ActionListener;

class Cells extends JPanel
{
    boolean[][] grid;
    int h, w;
    Cells(boolean[][] grid)
    {
       this.grid = grid;
        h = grid.length;
        w = grid[0].length;
    }
    {
        setBounds(50, 20, 961, 620);
        setBackground(Color.DARK_GRAY);
    }
    @Override
    public void paintComponent(Graphics g)
    {
        super.paintComponent(g);
        Graphics2D g2 = (Graphics2D) g;
        //g2.setColor(Color.BLUE);

        for (int x = 0; x < w * 10; x+=10)
        {
            for (int y = 0; y < h * 10; y+=10)
            {
                if (grid[y/10][x/10])
                {
                    g2.setColor(Color.BLUE);
                    g2.fillRect(x, y, 10, 10);
                }
                else
                {
                    g2.setColor(Color.gray);
                    g2.drawRect(x, y, 10, 10);
                }
            }
        }
    }
}
public class GameOfLife extends JFrame
{
    public final int height = 60;
    public final int width = 90;

    Universe universe = new Universe(height, width, (int) Math.random(), "Random");

    Cells cells = new Cells(universe.getCurrentGeneration());

    JLabel generationLabel = new JLabel("Generation#" + universe.getGeneration());
    JLabel aliveLabel = new JLabel("Alive: " + universe.getAlive());

    JButton resetButton, speedUpButton, slowDownButton;
    JToggleButton playToggleButton;

    String[] items = {"random", "Glider", "Gun", "Spaceship", "Beacon", "Pulsar"}; //to be added
    JComboBox patterns = new JComboBox(items); //to be added

    ActionListener repaint = e ->
    {
        universe.moveToNextState();
        generationLabel.setText("Generation #" + universe.getGeneration());
        aliveLabel.setText("Alive: " + universe.getAlive());
        cells.grid = universe.getCurrentGeneration();
        repaint();
        setVisible(true);
    };

    int speed = 100;
    Timer timer = new Timer(speed, repaint);

    public GameOfLife()
    {
        setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        setSize(1000, 700);
        setResizable(false);
        setLocationRelativeTo(null);
        setLayout(null);
        setBackground(Color.darkGray);
        getContentPane().setBackground(Color.darkGray);

        generationLabel.setName("GenerationLabel");
        aliveLabel.setName("AliveLabel");
        resetButton = new JButton("Reset");
        resetButton.setName("ResetButton");
        playToggleButton = new JToggleButton("Pause");
        playToggleButton.setName("PlayToggleButton");
        speedUpButton = new JButton("Speed+");
        slowDownButton = new JButton("Speed-");

        add(cells);
        addLabels();
        addButtons();
        addFunctionality();

        timer.start();

        setVisible(true);
    }

    void addLabels()
    {
        JPanel labels = new JPanel()
        {
            {
                setBounds(50, 636, 200, 40);
                setLayout(new BoxLayout(this, BoxLayout.Y_AXIS));
                setBackground(Color.DARK_GRAY);
                generationLabel.setForeground(Color.LIGHT_GRAY);
                aliveLabel.setForeground(Color.LIGHT_GRAY);
                add(generationLabel);
                add(aliveLabel);
            }
        };
        add(labels);
    }
    void addButtons()
    {
        JPanel buttons = new JPanel()
        {
            {
                setBounds(250, 636, 500, 40);
                setLayout(new BoxLayout(this, BoxLayout.X_AXIS));
                setBackground(Color.DARK_GRAY);
                resetButton.setForeground(Color.darkGray);
                playToggleButton.setForeground(Color.darkGray);
                speedUpButton.setForeground(Color.darkGray);
                slowDownButton.setForeground(Color.darkGray);

                resetButton.setBackground(Color.LIGHT_GRAY);
                playToggleButton.setBackground(Color.LIGHT_GRAY);
                speedUpButton.setBackground(Color.LIGHT_GRAY);
                slowDownButton.setBackground(Color.LIGHT_GRAY);

                add(resetButton);
                add(playToggleButton);
                add(speedUpButton);
                add(slowDownButton);
            }
        };
        add(buttons);
    }
    void addFunctionality()
    {
        playToggleButton.addActionListener(e ->
        {
             if (playToggleButton.getText().equals("Play") && !timer.isRunning())
             {
                 timer.restart();
                 playToggleButton.setText("Pause");
             }
             else if (playToggleButton.getText().equals("Pause") && timer.isRunning())
             {
                 timer.stop();
                 playToggleButton.setText("Play");
             }
        });
        speedUpButton.addActionListener(e ->
        {
            if (speed == 0)
            {}
            else
                timer.setDelay(speed -= 50);
        });
        slowDownButton.addActionListener(e -> timer.setDelay(speed += 50));
        resetButton.addActionListener(e -> universe.reset(height, width, (int) Math.random()));
    }
    public static void main(String[] args)
    {
        new GameOfLife();
    }
}

Any comment, even if it was on one small issue with my code will be appreciated. The purpose of the code is to simulate Conway's game of life based on an initial random state, though I am planning to add a JComponent that allows the user to choose an initial state. The evolution of the universe is displayed as an animation with options for the user to pause, resume, reset the universe, and adjust the speed of the animation. The code does all of that, but I am not sure about it's performance, look-and-feel, conciseness, or readability.

Answer 1

The first thing that I notice when looking at the code is something that directly stings in my eyes: Formatting. The code is is not formatted according to the SUN or Google Code Conventions (which are largely identical). The opening curly braces are misplaced, and the else is misplaced. Also, the indentation is inconsistent. Some blocks are indented 2, some 3, some 4 spaces. The visual appearance of Java code should be an indentation of 4 spaces.

The second thing which I noticed right away is that there are no tests.

The third thing is that your implementation of Game of Life is limited to a finite universe: the width and height of the universe must be known upfront. It is possible to create an implementation that supports an infinite universe. And if you like Game of Life (I do), you may find it a very interesting and enlightening challenge to attempt yourself at such an implementation.

Use descriptive names for variables. In your code, unless i really is just an anonymous counter, it's better to use x and y or row and col. And these names should be consistent throughout the code. I see sometimes i and j, sometimes p and m, sometimes row and column in your code.

Same goes for width and height, for which I sometimes see N and M instead.

The this qualifier should be omitted unless it is necessary to resolve ambiguity or otherwise communicate intent.

Your "main" class is a component that extends JFrame. I know that code examples in books and tutorials are full of examples like that. But this is bad practice and not proper OO: It violates the LSP (Liskov Substitution Principle; Barbara Liskov justifiedly received a Turing award for coming up with this) because your main class cannot be reused the same way as a JFrame. In layman's terms, subclasses should always represent adequate substitutes for their superclasses. And it is not necessary at all to extend JFrame. You can just fine do something like JFrame frame = new JFrame() and then call its methods.

Besides, GameOfLife is a bad name for something that extends JFrame. It should be possible to make an educated guess about the class hierarchy from the class name. The class name GameOfLife has nothing in it that suggests that it is a JFrame, nor anything that suggests that this is the class with the main() method.

Same goes for Cells. The name Cells does not suggest to the reader that this class is a UI component.

The superclass for Cells should be JComponent, not JPanel. The purpose of a JPanel is that you can set its layout manager and add components. Using JPanel instead of JComponent is again an LSP violation.

You could use more and thus smaller methods. This would allow you to have less duplication, more code reuse, and thus also fewer errors. For example, look at the constructor Universe(). It includes a section of code that initializes the entire universe with random bits. The method reset() does the same. You could extract this into a method randomize(), like this:

    public void randomize(int height, int width, int seed) {
        for (int y = 0; y < h; y++) {
            for (int x = 0; x < h; x++) {
                currentGeneration[y][x] = random.nextBoolean();
            }
        }
    }

You could call this randomize() method from both, reset() and Universe().

You might want to prefer method references over anonymous lambdas if you do not need Java's half-assed closures (access to variables of the enclosing method; in Java half-assed because they must be effectively final). This makes your code cleaner.

Fields which are initialized with a field initializer or with an assignment in the constructor but never assigned again should be final. If you want to write really good code, then most of your data will be final.

calculateNeighbors() is always called with currentGrid as its first argument. Remove the argument and make calculateNeighbors() an instance method. Same for calculateAlive().

In calculateNeighbors(), the code

            if (r < 0)
                r = N - 1;
            if (r > N - 1)
                r = 0;
            if (c < 0)
                c = M - 1;
            if (c > M - 1)
                c = 0;

can be simplified significantly:

            r = (r + N) % N;
            c = (c + M) % M;

(x + r) % r is the general formula to ensure for x ∈ ℤ, r ∈ ℕ that 0 <= x < r. Besides, this simplification will ensure the expected (torus universe) behavior in case you want to support a ruleset with a neighbor distance > 1.

In the methods generateNthGeneration(), X (uppercase) is used as a parameter name. This is misleading: A single uppercase letter is expected to be a type or a constant, but not a (in this case parameter) variable.

In your repaint, you have this code:

cells.grid = universe.getCurrentGeneration();

The class Cells should be able to render the correct generation without having another class (GameOfLife) helping with it. For that, class Cells should directly refer to class Universe, not its grid[][].

Overall, look out for duplication and remove it.

Also, look out for misplaced responsibility. You can detect misplaced responsibility by using ., especially when using it multiple times. There is a princple called Law of Demeter that can help you with identifying misplaced responsibility. You will notice that when you fix misplaced responsibility by moving things in the right places, that lines become shorter.

Remove unused code. Method getNextGeneration() is never used.

In method generateNextGeneration(), you may want to use a separate class to determine whether a cell survives or is born. This would allow you to easily implement other rulesets. Conway's Game of Life is B3/S23. Another popular ruleset is Highlife, B36/S23. The design pattern to do that is called Strategy.