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

First Java program critique (Game of Life)

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

Problem

As a school project in my navigator 12 class, I've decided to try and learn Java. I thought that I'd try and make the Game of Life, because it would be a good way to start learning Java (watched a bunch of Java tutorials).

Could I get your feedback on this program I wrote? I would like input on if everything is inputted properly and what I should've done differently, and if there is more effective and efficient ways to do a certain task. Also, just any type feedback would be helpful.

I made the game so the sides of the board would be connected to each other, and so the simulation would be contained(it will loop to the other side). I also have a slider to change the speed the game iterates. Other features are randomizing the board, and play and stop buttons. You can also, change the size of the board, with inputting the size you want it to generate.

Main

public class Main {

    public static void main(String[] args) {

        new Settingsboard();

    }

}


Settings

```
public class Settingsboard extends JFrame{

JFrame Frame2 = new JFrame();
JLabel label1, label2, value1, value2;
JTextField x_Value, y_Value;
JButton button1, button2, button3, clear;
static JCheckBox random;
int run = 0;
static JSlider howManyTimes;
int delay = 1000; // 1000 ms == 1 second
javax.swing.Timer myTimer = new javax.swing.Timer(delay,
new MyTimerActionListener());

public Settingsboard(){

Frame2.setSize(400,400);
JPanel thePanel = new JPanel();
//JPanel thePanel2 = new JPanel();

label1 = new JLabel("Set Size Of Board: ");
thePanel.add(label1);

value1 = new JLabel(" Width: ");
thePanel.add(value1);
x_Value = new JTextField("", 5);
thePanel.add(x_Value);

value2 = new JLabel(" Height: ");
thePanel.add(value2);
y_Value = new JTextField

Solution

So there's a lot to go though in here. I think that as a beginner project you can be proud of your code... there are a number of things I would change, but, on the whole, it is systematic, logical, and well-formatted. In my opinion this is far more preferable than 'clever' code that is hard to read and understand...

So, choosing some things that you should work on:

  • It appears you have used a code-editor to help you (intelliJ maybe, or eclipse?). This is a good thing, because it will help you to debug your programs, and it can help you refactor with fewer errors.



  • I would start by renaming some of your J* components... label1, label2, button1, etc. do not help much.



  • Frame2 should have a lower-case 'f', and why do you need two JFrames? Every time you have Frame2 you should instead have this (the SettingsBoard)



  • Using spaces in Labels to get the justification right, is wrong: value1 = new JLabel(" Width: "); should be solved by using the correct size and alignment settings in your LayoutManager



-
JCheckBox random and JSlider should not be static, should they?

-
in your Board class you have multiple JFrames as well. The Board class itself is a JFrame, so use it.

-
In the Logic method, there is a huge amount of duplicate code.... there's good ways to reduce it... I'll describe a system which I would not expect you (as a beginner) to think of, but it's neat, concise, and really makes things easy...

Your goal is to 'wrap' the game of life, so cells on the left margin are impacted by cells on the right, and top and bottom too.... You can use the modulo operator.... and a 'width' (or height) offset. And this way you an eliminate 90% of your code. Consider this function:

private static final int[][] offsets = {
        {-1, -1}, // up left
        {-1,  0}, //    left
        {-1,  1}, // dn left
        { 0, -1}, // up 
        { 0,  1}, // dn
        { 1, -1}, // up right
        { 1,  0}, //    right
        { 1,  1}  // dn right
    }

private static final int getAliveNeighbors(int x, int y, int width, int height, Boolean[][] boolboard) {

    int alive = 0;
    // look around us.... how many cells are alive
    for (int[] offset : offsets) {
        if (boolboard[(x + offset[0] + width) % width][(y + offset[1] + height) % height]) {
            alive++;
        }
    }
    return alive;
}


The above function is 'clever'. It adds the width to the coordinate, and then gets the remainder when dividing by the width. For example, if your grid is 10x10, and you are looking for life at cell 0,0, then you want to check for life in the cell to the left (-1, 0) then you really want to look in cell (9, 0), which is ((0 + 10 - 1) % 10 == 9 , 0)

Now we have an 'easy' function for counting the amount of life, you can use that to massively simplify your 'Logic' code.

Code Snippets

private static final int[][] offsets = {
        {-1, -1}, // up left
        {-1,  0}, //    left
        {-1,  1}, // dn left
        { 0, -1}, // up 
        { 0,  1}, // dn
        { 1, -1}, // up right
        { 1,  0}, //    right
        { 1,  1}  // dn right
    }

private static final int getAliveNeighbors(int x, int y, int width, int height, Boolean[][] boolboard) {

    int alive = 0;
    // look around us.... how many cells are alive
    for (int[] offset : offsets) {
        if (boolboard[(x + offset[0] + width) % width][(y + offset[1] + height) % height]) {
            alive++;
        }
    }
    return alive;
}

Context

StackExchange Code Review Q#36172, answer score: 12

Revisions (0)

No revisions yet.