4
\$\begingroup\$

I wrote a program for Conway's Game Of Life in Java. Are there any improvements I could make?

main class:

package main;

import java.awt.Color;
import java.awt.Graphics2D;
import java.awt.image.BufferStrategy;

public class GameOfLife {
    
    static Board board;
    static Window window;
    static Input input = new Input();
    static BufferStrategy bs;
    
    //board and window setup
    public static void start() {
        window = new Window("game", 800, 500);
        board = new Board(window.getCanvas().getWidth(), window.getCanvas().getHeight());
        board.setUp();
    }
    
    //renders the board
    public static void render() {
        bs = window.getCanvas().getBufferStrategy();
        if(bs == null) {
            window.getCanvas().createBufferStrategy(2);
        }
        bs = window.getCanvas().getBufferStrategy();
            
        window.getCanvas().setBackground(Color.black);
        window.requestFocusInWindow();
        Graphics2D g2d = (Graphics2D) bs.getDrawGraphics();
        
        g2d.clearRect(0, 0, window.getCanvas().getWidth(), window.getCanvas().getHeight());

        g2d.setColor(Color.white);
        for (int i = 0; i < board.getBoard().length; i++) {
            for (int j = 0; j < board.getBoard()[0].length; j++) {
                if (board.getState(i, j, board.getBoard())) {
                    g2d.fillRect(i * board.getCellSize(), j * board.getCellSize(), board.getCellSize(),
                            board.getCellSize());
                }
            }
        }

        g2d.dispose();
        bs.show();
    }
    
    //timing for framerate
    public static void pause(long time) {
        try {
            Thread.sleep(time);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }

    //main loop
    public static void main(String[] args) {
        start();
        while(true) {
            long start = System.nanoTime();
            
            if(!input.getPaused()) {
                board.updateBoard();
            }
            render();
            
            long deltatime = (System.nanoTime() - start) / 1000000;
            
            if((16 - deltatime) >= 0) {
                pause(16 - deltatime);
            }
        }
    }

}

board class:

package main;

import java.util.Random;

public class Board {

    private boolean[][] board;
    private boolean[][] newboard;
    int cellsize = 4;
    Random random = new Random();
    
    public Board(int width, int height) {
        board = new boolean[width/cellsize][height/cellsize];
        newboard = new boolean[width/cellsize][height/cellsize];
    }
    
    //getters and setters
    public boolean[][] getBoard() {
        return this.board;
    }
    
    public boolean getState(int x, int y, boolean[][] board) {
        if(x < 0 || y < 0 || x > board.length-1 || y > board[0].length-1) {
            return false;
        }
        return board[x][y];
    }
    
    public int getCellSize() {
        return this.cellsize;
    }
    
    //counts number of alive cells next to cell
    public int countNeighbours(int x, int y) {
        int count = 0;
        
        for(int i = x-1; i <= x+1; i++) {
            for(int j = y-1; j <= y+1; j++) {
                if(i != x || j != y) {
                    if (getState(i, j, board)) {
                        count++;
                    }
                }
            }
        }
        
        return count;
    }
    
    //returns state of cell in the next generation
    public boolean getNewState(int x, int y) {
        int count = countNeighbours(x, y);
        boolean state = getState(x, y, board);
        
        if(state && count > 1 && count < 4) {
            return true;
        }
        else if(!state && count == 3) {
            return true;
        }
        
        return false;
    }
    
    //changes states to those of the next generation
    public void updateBoard() {
        for(int i = 0; i < board.length; i++) {
            for(int j = 0; j < board[i].length; j++) {
                newboard[i][j] = getNewState(i, j);
            }
        }
        
        for(int x = 0; x < newboard.length; x++) {
            for(int y = 0; y < newboard[x].length; y++) {
                board[x][y] = getState(x, y, newboard);
            }
        }
    }
    
    //assigns every cell a random state
    public void setUp() {
        for(int i = 0; i < board.length; i++) {
            for(int j = 0; j < board[i].length; j++) {
                board[i][j] = random.nextBoolean();
            }
        }
    }
}

window class:

package main;

import java.awt.Canvas;
import java.awt.Point;

import javax.swing.JFrame;

public class Window extends JFrame {
    
    private static final long serialVersionUID = 1L;
    private Canvas canvas;
    static Input input = new Input();

    //window setup
    public Window(String title, int width, int height) {
        setSize(width, height);
        setTitle(title);
        setDefaultCloseOperation(EXIT_ON_CLOSE);
        setResizable(false);
        setLocation(new Point(0, 0));
        setVisible(true);
        
        canvas = new Canvas();
        canvas.setSize(width, height);
        canvas.setFocusable(true);
        
        add(canvas);
        addKeyListener(input);
    }
    
    //getters and setters
    public Canvas getCanvas() {
        return canvas;
    }
            
    public void setCanvas(Canvas canvas) {
        this.canvas = canvas;
    }

}

input class:

package main;

import java.awt.event.KeyEvent;
import java.awt.event.KeyListener;

public class Input implements KeyListener {
    
    private static boolean paused;

    public Input() {
        
    }

    @Override
    public void keyTyped(KeyEvent e) {

    }

    @Override
    public void keyPressed(KeyEvent e) {
        if(e.getKeyCode() == KeyEvent.VK_SPACE) {
            paused = !paused;
        }
    }

    @Override
    public void keyReleased(KeyEvent e) {

    }
    
    //getters and setters
    public boolean getPaused() {
        return paused;
    }

}
\$\endgroup\$
0

2 Answers 2

1
\$\begingroup\$

Welcome to the site! This looks good so far, here's a couple of smaller tips:

Board.java

  • The cellsize doesn't change, so why not make it a constant like this:
    public static final int CELLSIZE = 4;
    This also makes the getter obsolete, as you can access that field using Board.CELLSIZE

  • getNewState() uses a boolean state. Renaming that to something like isAlive makes the if-clauses a bit easier to read.

  • You only use the Random for the board setup, so there's no reason for it to be a (package-private) attribute of Board. Move it into setUp().

Input.java

  • Boolean getters are almost always called isX() instead of getX(), so getPaused() should be renamed to isPaused()

  • Make isPaused() a static method. This way, you can access the method using Input.isPaused() in GameOfLife.java without having to do static Input input = new Input(); This also removes the unnecessary static member and the instance of Input.

Window.java

  • setLocation can also be called with two ints instead of a Point. This looks a bit more clean and saves the import

  • Tip: you can center the window on the screen using setLocationRelativeTo(null)

  • setVisible(true) is usually the last method called in the constructor. If you had a slower PC or a more complicated GUI, making the window visibile too early results in the user seeing the window build itself up and/or move around/resize.

  • input doesn't need to be a static attribute of the Window class. Writing
    addKeyListener(new Input());
    works just as well.

GameOfLife.java

  • Move the BufferStrategy-related things into start(). I don't think that you need to call these every frame. I'd write something like this:
    public static void start() {
       ...
        board.setUp();
    
        if(window.getCanvas().getBufferStrategy() == null) {
            window.getCanvas().createBufferStrategy(2);
        }
        bs = window.getCanvas().getBufferStrategy();
    }
  • (16 - deltatime) >= 0 is a bit weird to read, deltatime <= 16 is more intuitive. Also, if deltatime == 16, you'll sleep for 0 seconds; I'd put a < instead of a <= there.

  • 16 is a magic number. Don't use these, instead consider doing something that's more descriptive:

private static final int FPS = 60
private static final int FRAMETIME_MS = 1000/FPS;

General tip

Consider the visibility of your attributes. Make everything private that doesn't need to be seen or has a getter and setter. Also consider the scope: Does this variable need to be an attribute or do you only need it for one method? If so, move it in there.

Something about the Board/Window classes seems a bit off to me. I think that some things like the window.getCanvas(). ... could be cleaned up by making Board extend Canvas or making Board an attribute of Window, or even both. Perhaps I'll look at it again later.

\$\endgroup\$
1
\$\begingroup\$

Input doesn't need to be a separate source file when you can use an anonymous inner class that implements KeyListener. When you do this, it becomes easier to put the variable paused where it belongs (i.e. not in the class handling the input). See my edits below for how to do it.

Similar thing with Window - it doesn't need to be a separate file, and we don't even need to keep a reference to the JFrame for the moment, though we might want to later.

You can move the cell size out of the Board class - the class with the game logic shouldn't be concerned with how it gets drawn. In fact, Board is actually the game logic itself, maybe that class should be renamed to GameOfLife and your main class renamed something else.

Though the code still does mostly the exact same stuff, I have made a bunch of other simplifying changes, so read through my version carefully if you are interested.

package main;

import java.awt.Canvas;
import java.awt.Color;
import java.awt.Graphics;
import java.awt.event.KeyEvent;
import java.awt.event.KeyListener;
import java.awt.image.BufferStrategy;

import javax.swing.JFrame;

public class GameOfLifeDemo {

    private static final long FRAME_TIME = 1000 / 24;

    GameOfLife game;
    boolean paused = false;

    Canvas canvas = new Canvas();
    BufferStrategy bufferStrategy;
    int cellSize;

    GameOfLifeDemo(String title, int width, int height, int cellSize) {
        this.cellSize = cellSize;
        this.game = new GameOfLife(width / cellSize, height / cellSize);

        canvas.setBackground(Color.black);
        canvas.setSize(height, width);
        canvas.setFocusable(true);
        canvas.addKeyListener(new KeyListener() {
            @Override
            public void keyTyped(KeyEvent e) {
            }

            @Override
            public void keyPressed(KeyEvent e) {
                if (e.getKeyCode() == KeyEvent.VK_SPACE) {
                    paused = !paused;
                }
            }

            @Override
            public void keyReleased(KeyEvent e) {
            }
        });

        JFrame window = new JFrame();
        window.setSize(width, height);
        window.setTitle(title);
        window.setResizable(false);
        window.add(canvas);
        window.setVisible(true);

        canvas.createBufferStrategy(2);
        bufferStrategy = canvas.getBufferStrategy();
        canvas.requestFocusInWindow();
    }

    private void render() {
        Graphics graphics = bufferStrategy.getDrawGraphics();
        graphics.clearRect(0, 0, canvas.getWidth(), canvas.getHeight());
        graphics.setColor(Color.white);
        for (int i = 0; i < game.getWidth(); i++) {
            for (int j = 0; j < game.getHeight(); j++) {
                if (game.getState(i, j)) {
                    graphics.fillRect(i * cellSize, j * cellSize, cellSize, cellSize);
                }
            }
        }
        graphics.dispose();
        bufferStrategy.show();
    }

    public void run() throws InterruptedException {
        while (true) {
            long start = System.nanoTime();
            if (!paused) {
                game.tick();
                render();
            }
            long deltatime = (System.nanoTime() - start) / 1000000;

            if ((FRAME_TIME - deltatime) > 0) {
                Thread.sleep(FRAME_TIME - deltatime);
            }
        }
    }

    public static void main(String[] args) throws InterruptedException {
        new GameOfLifeDemo("Game o' life", 800, 600, 4).run();
    }

}
package main;

import java.util.Random;

public class GameOfLife {

    private final static Random RANDOM = new Random();

    private final int height;
    private final int width;
    private boolean[][] board;
    private boolean[][] nextboard;

    public GameOfLife(int width, int height) {
        this.height = height;
        this.width = width;
        board = new boolean[width][height];
        nextboard = new boolean[width][height];
        for (int i = 0; i < width; i++) {
            for (int j = 0; j < height; j++) {
                board[i][j] = RANDOM.nextBoolean();
            }
        }
    }

    public int getWidth() {
        return width;
    }

    public int getHeight() {
        return height;
    }

    public boolean getState(int x, int y) {
        return board[x][y];
    }

    private int countNeighbours(int x, int y) {
        int count = 0;
        for (int i = x - 1; i <= x + 1; i++) {
            for (int j = y - 1; j <= y + 1; j++) {
                if (i == x && j == y) {
                    continue;
                }
                if (i < 0 || i >= width || j < 0 || j >= height) {
                    continue;
                }
                if (board[i][j]) {
                    count++;
                }
            }
        }
        return count;
    }

    private boolean nextCellState(int x, int y) {
        int neighbourCount = countNeighbours(x, y);
        if (board[x][y] && neighbourCount > 1 && neighbourCount < 4) {
            return true;
        } else if (!board[x][y] && neighbourCount == 3) {
            return true;
        }
        return false;
    }

    public void tick() {
        for (int i = 0; i < board.length; i++) {
            for (int j = 0; j < board[i].length; j++) {
                nextboard[i][j] = nextCellState(i, j);
            }
        }

        /*
         * Just swap the references to start using the newboard arrays as the current
         * board arrays. The old arrays will be used to construct the next iteration.
         */
        boolean[][] temp = board;
        board = nextboard;
        nextboard = temp;
    }

}
\$\endgroup\$
1
  • \$\begingroup\$ +1 especially for recommending to move the cell size out of the game logic. \$\endgroup\$ Jul 29, 2021 at 16:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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