HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Full-color clone of Conway's Game of Life, with a decent GUI

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
decentfullwithcolorconwaygamelifeguiclone

Problem

I wrote this to learn JavaFX, and as an excuse to re-make the Game of Life. This is the most complex GUI I've ever written, so I'd like feedback mainly on it, but I'll welcome any criticism!

My version differs from the original in that cells inherit their neighbour's color when they're born, which leads to interesting "war" scenarios.

It's also technically "1-player", as the user can intervene by painting new live cells right onto the field to affect the evolution.

A sample:

What I'd like commented on mainly:

-
All of the settings that the controls affect are global; they're (private) instance members of the application class. I know that in general, this is bad practice, but I'm not sure how else to set it up. If I have an event handler that needs to change a setting, short of threading every setting through the handlers and the main-loop, I'm not sure how else it could be accomplished.

-
The general set-up of the controls in the code. Is there a standard way of creating/setting up controls? Here, I've segregated set up of controls into functions based on which bar they belong to. It works, but I can see it getting out of control with more controls added.

-
The GOL class (Environment) is actually faster than I expected, but it gets quite slow at around 500x500 cells. Is there anything I can do to help speed it up?

-
Anything regarding layout of the GUI.

Note that currently this isn't a "real" GOL clone in that it's not infinite. When a cell exits the screen, it's no longer "seen", and is erased the next generation. This leads to the corruption of classic patterns if they collide with the end of the world.

GameOfLife.java:

```
package gameOfLife;

import static utils.Utils.clearCanvas;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Random;

import javafx.application.Application;
import javafx.event.ActionEvent;
import javafx.event.Event;
import javafx.event.EventHandler;
import javafx.scene.Scene;
imp

Solution

Naming

Manifest constants typically use _ for space in Java. So NANOSPERSECOND for example should be NANOS_PER_SECOND.

Also I believe that the name MainLoop is quite misleading as a name because the class isn't actually your main loop but rather a timing helper.

Avoid unnecessary conversions

You don't need the double precision here:

public final static long IDEALFRAMERATENS = (long)(1 / 60.0 * NANOSPERSECOND);


just swap the operations:

public final static long IDEALFRAMERATENS = NANOSPERSECOND / 60;


The result is the same because you're truncating anyway.

Model View Controller (non-)separation

Your class GameOfLife mixes all three of model, view and controller. It is very desirable to keep these separate. What I would do is to separate everything that has to do with the game simulation: isPaused, cellsWide/High, the mainloop etc and put into a new class GameOfLifeSimulator. Next I would separate the UI design from the class and put it into an FXML document that describes the UI. Then I would have a separate controller class for the FXML document and leave the application class just as a pretty basic launcher.

If what I just said sounds like gibberish, have a look here: Model View Controller.

Using FXML would allow you to create your event handlers as member functions in the controller instead. So you would have:

@FXML
public void startStopSimulation(){
    isPaused = !isPaused;
}


and in the fxml you would have an attribute saying onAction="startStopSimulation". To link the button action to the method in your controller.

This here will get you started with FXML.

Use properties

JavaFX controls are built on the powerful concept of observables, properties and bindings. An observable is a class that wraps an object that allows you to listen to changes in the object. A property is a value that is observable. A binding is a way to create an expression that is bound to the values of properties, using the fact that they are observable. So if a property changes, so does all bindings that depend on it. And finally, you can bind the value of a property to a binding so you can effectively link properties to each other and create arbitrary expressions of your properties and bind those expressions to other properties. More info here.

For example you could do the following:

NumberBinding averageScaleFactor = Bindings.divide(
    canvas.widthProperty().add(canvas.heightProperty()),
    cellsWideSlider.valueProperty().add(cellsHighSlider.valueProperty()));


To create a binding, a kind of numerical expression that can be evaluated.

And whenever you need the scale factor simply call: averageScaleFactor.get(). The result is cached and automatically updated as soon as either the canvas or the sliders change. So the above snippet will eliminate the getAverageScalingFactor method.

Going with the Model-View-Controller idea, your "model", the GameOfLifeSimulator would have: IntegerProperty cellsWide/High; that you would bind to the value of the sliders of your UI ("view") in your "controller". And they would automatically be kept in sync.

Avoid allocating new objects in your game loop.

I see that you frequently allocate new Position elements in your game code. While memory allocation is cheap in Java, frequently allocating objects and throwing them away like you're doing will cause a significant strain on the GC and can cause your game to appear "jittery". I would advice to allocate the position objects once and re-use them where it makes sense.

For example here:

private SpeciesFreqs getNeighborsOf(int x, int y, int range) {
    speciesFreqs.reset();

    for (int checkY = y - range; checkY  pos = new Position(checkX, checkY);
            S cellSpecies = liveCells.get(pos);

            if (cellSpecies != null && !(checkX == x && checkY == y)) {

                speciesFreqs.add(cellSpecies);
            }
        }
    }


you allocate a new position for each of the $$4\cdot range^2$$ elements checked when you could get away with simply allocating pos once at the top and re-using it.

Or here:

public S getCellSpecies(int x, int y) {
    return liveCells.get(new Position(x, y));
}


or here:

public void setCell(int x, int y, S species) {
    liveCells.put(new Position(x,y), species);
}


This whole allocation and disposal of Position objects seem to be centered around the fact that you're using a Map to represent your live-state of the game. This may also be a cause for your performance hit, every access you make to the map requires that you must calculate the hash-code of these positions over and over again.

It would probably be faster if you just used Integer as key and used a custom hash function like so x + MAX_WIDTH*y. The function is collision free and faster to calculate than the hashCode of your Position class. Also you avoid straining the GC.

Another thing that might be even faster is to sim

Code Snippets

public final static long IDEALFRAMERATENS = (long)(1 / 60.0 * NANOSPERSECOND);
public final static long IDEALFRAMERATENS = NANOSPERSECOND / 60;
@FXML
public void startStopSimulation(){
    isPaused = !isPaused;
}
NumberBinding averageScaleFactor = Bindings.divide(
    canvas.widthProperty().add(canvas.heightProperty()),
    cellsWideSlider.valueProperty().add(cellsHighSlider.valueProperty()));
private SpeciesFreqs getNeighborsOf(int x, int y, int range) {
    speciesFreqs.reset();

    for (int checkY = y - range; checkY <= y + range; checkY++) {
        for (int checkX = x - range; checkX <= x + range; checkX++) {
            Position<Integer> pos = new Position<Integer>(checkX, checkY);
            S cellSpecies = liveCells.get(pos);

            if (cellSpecies != null && !(checkX == x && checkY == y)) {

                speciesFreqs.add(cellSpecies);
            }
        }
    }

Context

StackExchange Code Review Q#107844, answer score: 11

Revisions (0)

No revisions yet.