2
\$\begingroup\$

I have this version of John Conway's Game Of Life in Java:

Frame class:

import java.awt.BorderLayout;
import java.awt.FlowLayout;
import java.awt.Image;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;

import javax.swing.ImageIcon;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JMenu;
import javax.swing.JMenuBar;
import javax.swing.JMenuItem;
import javax.swing.JSeparator;
import javax.swing.JToolBar;

public class Frame extends JFrame {
    private Board board;
    private BoardInterface boardInterface;

    private JMenuBar menuBar;
    private JMenu fileMenu, runMenu;
    private JMenuItem newMenuItem, clearMenuItem, randMenuItem, exitMenuItem, runMenuItem, pauseMenuItem;

    private JToolBar toolBar; 
    private JButton run, pause;
    private Image runToolbar, pauseToolbar;

    public Frame(Board board) {
        this.board = board;
        boardInterface = new BoardInterface(board);
        setTitle("The Game of Life");
        setSize(board.getWidth() * board.getMultiplier() + 5, board.getHeight() * board.getMultiplier() + 101);
        setResizable(false);
        setVisible(true);
        setDefaultCloseOperation(JFrame.DISPOSE_ON_CLOSE);
        add(boardInterface);
        setMenuBar();
        setToolbar();
    }

    public void setMenuBar() {
        menuBar = new JMenuBar();
        add(menuBar, BorderLayout.NORTH);

        fileMenu = new JMenu("File");
        menuBar.add(fileMenu);

        newMenuItem = new JMenuItem("New");
        fileMenu.add(newMenuItem);
        newMenuItem.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent e) {
                new Frame(new Board());
            }
        });

        fileMenu.add(new JSeparator());

        clearMenuItem = new JMenuItem("Clear Board");
        fileMenu.add(clearMenuItem);
        clearMenuItem.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent e) {
                board.setBoard(board.clearBoard());
            }
        });

        randMenuItem = new JMenuItem("Random Board");
        fileMenu.add(randMenuItem);
        randMenuItem.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent e) {
                board.setBoard(board.randomBoard());
            }
        });

        fileMenu.add(new JSeparator());

        exitMenuItem = new JMenuItem("Exit");
        fileMenu.add(exitMenuItem);
        exitMenuItem.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent e) {
                dispose();
            }
        });

        runMenu = new JMenu("Run");
        menuBar.add(runMenu);

        runMenuItem = new JMenuItem("Run");
        runMenu.add(runMenuItem);
        runMenuItem.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent e) {
                boardInterface.setIsActive(true);
            }
        });

        pauseMenuItem = new JMenuItem("Pause");
        runMenu.add(pauseMenuItem);
        pauseMenuItem.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent e) {
                boardInterface.setIsActive(false);
            }
        });
    }

    public void setToolbar() {
        toolBar = new JToolBar();
        toolBar.setLayout(new FlowLayout(FlowLayout.CENTER));
        toolBar.setFloatable(false);
        add(toolBar, BorderLayout.SOUTH);

        runToolbar = new ImageIcon("src/playToolbar.png").getImage().getScaledInstance(25, 25, Image.SCALE_SMOOTH);
        run = new JButton(new ImageIcon(runToolbar));
        toolBar.add(run);
        run.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent e) {
                boardInterface.setIsActive(true);
            }
        });

        pauseToolbar = new ImageIcon("src/pauseToolbar.png").getImage().getScaledInstance(25, 25, Image.SCALE_SMOOTH);
        pause = new JButton(new ImageIcon(pauseToolbar));
        toolBar.add(pause);
        pause.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent e) {
                boardInterface.setIsActive(false);
            }
        });
    }


    public static void main(String[] args) {    
        Frame frameInterface = new Frame(new Board());
    }
}

BoardInterface class:

import java.awt.Color;
import java.awt.Graphics;
import java.awt.Toolkit;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.MouseAdapter;
import java.awt.event.MouseEvent;

import javax.swing.JPanel;
import javax.swing.Timer;

public class BoardInterface extends JPanel implements ActionListener {
    Timer animation = new Timer(500, this);

    private Board board;    
    private boolean isActive;

    public BoardInterface(final Board board) {
        this.board = board;
        setBackground(Color.BLACK);

        addMouseListener(new MouseAdapter() {
            @Override
            public void mousePressed(MouseEvent e) {
                if (!isActive) {
                    board.getBoard()[e.getY() / board.getMultiplier()][e.getX() / board.getMultiplier()] = !board.getBoard()[e.getY() / board.getMultiplier()][e.getX() / board.getMultiplier()];
                }
                else {
                    Toolkit.getDefaultToolkit().beep();
                }
            }
        });
    }

    public void setIsActive(boolean toSet) {
        isActive = toSet;
    }

    @Override
    public void paintComponent(Graphics g) {
        super.paintComponent(g);
        for (int i = 0; i < board.getHeight(); i++) {
            for (int j = 0; j < board.getWidth(); j++) {
                g.setColor(board.getBoard()[i][j] ? Color.GREEN : Color.GRAY);
                g.fillRect(j * board.getMultiplier(), i * board.getMultiplier(), board.getMultiplier() - 1, board.getMultiplier() - 1);
            }
        }
        if (isActive) {
            animation.restart();
        }
        else {
            animation.stop();
            repaint();
        }
    }

    public void actionPerformed(ActionEvent e) {
        if (e.getSource().equals(animation)) {
            board.nextGeneration();
            repaint();
        }
    }
}

Board class:

import java.util.Random;

public class Board {    
    private boolean[][] board;  
    private int height, width, multiplier = 10;

    public Board() {
        this(new boolean[60][60]);
    }

    public Board(final boolean[][] board) {
        this.board = board;
        height = board.length;
        width = board[0].length;
    }

    public int getHeight() {
        return height;
    }

    public void setHeight(int n) {
        height = n;
    }

    public int getWidth() {
        return width;
    }

    public void setWidth(int n) {
        width = n;
    }

    public int getMultiplier() {
        return multiplier;
    }

    public void setMultiplier(int n) {
        multiplier = n;
    }

    public boolean[][] getBoard() {
        return board;
    }

    public void setBoard(boolean[][] n) {
        for (int i = 0; i < height; i++) {
            for (int j = 0; j < width; j++) {
                board[i][j] = n[i][j];
            }
        }
    }

    public boolean[][] randomBoard() {
        Random rand = new Random();
        boolean[][] randBoard = new boolean[height][width];
        for (int i = 0; i < height; i++) {
            for (int j = 0; j < width; j++) {
                randBoard[i][j] = rand.nextBoolean();
            }
        }
        return randBoard;
    }

    public boolean[][] clearBoard() {
        boolean[][] emptyBoard = new boolean[height][width];
        for (int i = 0; i < height; i++) {
            for (int j = 0; j < width; j++) {
                emptyBoard[i][j] = false;
            }
        }
        return emptyBoard;
    }

    public int max(int i, int j) {
        return i > j ? i : j;
    }

    public int countSurrounding(int a, int b) {
        int count = 0;
        int[][] surrounding = {{a - 1, b - 1},
                               {a - 1, b    },
                               {a - 1, b + 1},
                               {a    , b - 1},
                               {a    , b + 1},
                               {a + 1, b - 1},
                               {a + 1, b    },
                               {a + 1, b + 1}};
        for (int[] i: surrounding) {
            try {
                if (board[i[0]][i[1]]) {
                    count++;
                }
            }
            catch (ArrayIndexOutOfBoundsException e) {}
        }
        return count;
    }

    public void nextGeneration() {
        boolean[][] nextBoard = new boolean[height][width];
        for (int i = 0; i < height; i++) {
            for (int j = 0; j < width; j++) {
                nextBoard[i][j] = board[i][j];
            }
        }
        for (int i = 0; i < height; i++) {
            for (int j = 0; j < width; j++) {
                if (board[i][j] && !(countSurrounding(i, j) == 2 || countSurrounding(i, j) == 3)) {
                    nextBoard[i][j] = false;
                }
                else if (!board[i][j] && countSurrounding(i, j) == 3) {
                    nextBoard[i][j] = true;
                }
            }
        }
        board = nextBoard;
    }   
}

And I came here to ask the standard question: can anyone provide an honest code review? Thanks.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ That's a good start. To bump up performance a bit, rather than creating a new board for every time you create the next generation, you should instead create two boards. One will represent the current generation and the other to represent the next. Use the current board to generate the next generation. Once the next generation is calculated, swap it with the old "current" generation and repeat the process. Since the calculations of the states of the cells are completely independent of each other, there's plenty of opportunity to parallelize the process. \$\endgroup\$ May 26 '13 at 7:47
3
\$\begingroup\$

Frame

Important Issues

  • Exit just disposes. This is fine for extra spawned frames, but the last one should really exit, or you risk the JVM just hanging around.
  • UI creation should happen on the Event Dispatch Thread (EDT), in the main method, use SwingUtilities.invokeLAter() to surround the creation of the first Frame.
  • Have only BoardInterface know about the Board class, this way the Frame only directly depends on BoardInterface (this involves creating some delegate methods on BoardInterface).

Minor Issues

  • All widget fields can be converted to local variables.
  • The remaining fields can be made final.
  • I would use Actions instead of ActionListeners for the buttons and menu items. This way buttons and menu items that do the same thing can share the same action, and you can enable/disable an action according to context (e.g. disable run action when already running)
  • Get rid of String literals and magic numbers (replace by explaining constants).

BoardInterface

Minor Issues

  • Timer visibility modifier : set to private.
  • Rename to BoardPanel (Interface causes confusion with the language construct)
  • Use an (anonymous) inner class for the ActionListener
    • this avoids exposing an extra public method, that is useless to normal clients
    • this makes the if clause that checks the source superfluous
  • do not increase visibility of the overridden method paintComponent(), keep it protected.

Board

Imortant Issues

  • Do not catch ArrayIndexOutOfBoundsException, check indices before accessing them.
  • Multiplier belongs on BoardInterface. Move it there.
  • Hide the internal array representation.
    • all you really need is methods to toggle, and query cells.

Minor Issues

  • Replace manual array copy with System.arrayCopy()
  • Remove unused methods
  • make countSurrounding() private
\$\endgroup\$
2
  • \$\begingroup\$ I think it's correct to dispose the frames. If the application is hanging around after frame disposal, there's a bug, and using System.exit() or WindowConstants.EXIT_ON_CLOSE is just a cover-up for that bug. Agreeing with everything else, so got my +1, especially for Action instead of ActionListener. \$\endgroup\$ Mar 17 '15 at 8:24
  • \$\begingroup\$ @ChristianHujer Note that I'm saying disposing is fine. Just that after closing the last frame the application does not end. There's no code in place to actually exit. \$\endgroup\$
    – bowmore
    Mar 17 '15 at 12:01
1
\$\begingroup\$

I am not a Java pro, so not everything I say now must be correct, but here is my review:

First of all: You did a good job when it comes to encapsulating. Nearly all of your methods are just a few lines, so every method really only does what it is supposed to do. Furthermore, your code is a good example that code, if it is well written, needs no comments to explain WHAT is done. Because you should see from the code itself what is done.

So here come the negative things:

  • Also code does not necessarily need comments which tell WHAT is done, it needs (in my opinion) comments which tell WHY something is done. So I miss some comments in your code, for example just a short explanation how the classes work together, or, for example because this is not clear to me, why the height and width of the board is not set in the constructor.
  • You should not use "magic numbers". That means do not hardcode sizes or positions or something similar. For example in the Board constructor you make an array with size [60][60]. That is fine. But if you want to change the size later, then you have too look where you wrote the size. Better would be if you wrote the values as a constant in the class. So you can easily find and change it, and, as additional extra, use it everywhere in the class. Maybe you come to a point where you need this. The same advice is for the part where you create you JFrame.

    setSize(board.getWidth() * board.getMultiplier() + 5, board.getHeight() * board.getMultiplier() + 101); You should write these numbers as constants. If you change the size of your frame, then you will have to change this numbers as well. So you can easier find them as constants. Or even better, if this is possible: Calculate the size of your frame dynamically.

  • In your board class, you have getter and setter for the board's height and width. But why is the setter for the width and height public? You use the values of height and width in every update step. So what would happen if they were changed somewhere out of the class? My advise, when it comes to array sizes, is: Do not use setter for this values. When the array is already created, then nothing may change the borders. Only change the borders when the array is changed as well.

  • In the method setBoard you assume that the array which you get has the same size (or bigger) than the array you are holding in your object. You should make a quick check if the passed array has the same size (or is bigger, what would be okay but would make no sense). Otherwise your program will crash when the passed array is smaller. It is also not very clear what this method should do. Should the passed array be the new array? Then you should adjust the size of your intern array. Or should just the values passed like you do it now? Then you have to check the size of the array.

  • You often use very short, inexpressively variable or parameter names. This is okay in a nested for loop (although I would prefer iX and iY instead of i and j, but that is just a habit). But it is not okay for parameters. For example you use "n" as parameter in "setMultiplier" and you use "n" as parameter in "setBoard". They differ in type and in what they are supposed to be used for. You parameter names should describe what they are for. This does not need to be a long name, just something like "new_Board_Array" or "multiplier_Value". Imagine you have a method with more than one parameter and you use your naming convention: public int foo(int n, bool[][] m, float k). Then it would be messy inside your method.

This is all what I noticed. Just out of curiosity: Do you want to add more functionality to your game of life program? For example when I wrote a game of life program I added some functionality like saving the curent field or going backward in the game to see older fields. You can also implement some functionality like speeding up the game or having custom field sizes instead of fixed 60*60.

Hope this helps you a bit.

\$\endgroup\$
0
\$\begingroup\$

It is pretty good. I agree with the comments of @MOrgenstern.

There is one big problem however: you are frequently trying to fetch elements outsize the arrays range in countSurrounding(), and you catch the exceptions. You should never use exceptions to deal with standard conditions since they kill performance. You could fix that in many ways. One way which would add efficiency would be to precompute the neighbors of each cell at creation.

\$\endgroup\$
2
  • \$\begingroup\$ What I would see in typical implementations would be "border" cells adding extra rows/columns on the edges that do not contribute to the simulation. That way you can safely ignore those cells and spending time checking conditions or handling exceptions when creating the next generation. \$\endgroup\$ May 26 '13 at 7:38
  • \$\begingroup\$ That's possible too, but I don't like it so much since you'll have to add non-standard code elsewhere to ignore the extra rows. For example when generating the new active cells and when printing the board and maybe in many other places. I suspect it will end up being more complex than just dealing correctly with the neighbor counting. \$\endgroup\$
    – toto2
    May 26 '13 at 12:20

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.