4
\$\begingroup\$

I have this Board class:

import java.awt.Color;
import java.awt.Graphics;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.MouseEvent;
import java.awt.event.MouseListener;

import java.util.Random;

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

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

    private static boolean[][] emptyBoard = {{false, false, false, false, false, false, false, false, false, false},
                                             {false, false, false, false, false, false, false, false, false, false},
                                             {false, false, false, false, false, false, false, false, false, false},
                                             {false, false, false, false, false, false, false, false, false, false},
                                             {false, false, false, false, false, false, false, false, false, false},
                                             {false, false, false, false, false, false, false, false, false, false},
                                             {false, false, false, false, false, false, false, false, false, false},
                                             {false, false, false, false, false, false, false, false, false, false},
                                             {false, false, false, false, false, false, false, false, false, false},
                                             {false, false, false, false, false, false, false, false, false, false}};

    private boolean[][] board;
    private boolean isActive = false;
    private int height;
    private int width;
    private int multiplier = 55;

    private JButton button1;
    private JButton button2;
    private JButton button3;

    public Board() {
        this(emptyBoard);
    }

    public Board(final boolean[][] board) {
        this.board = board;
        height = board.length;
        width = board[0].length;
        setBackground(Color.black);
        button1 = new JButton("Run");
        add(button1);
        button1.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent e) {
                isActive = !isActive;
                button1.setText(isActive ? "Pause" : "Run");
            }
        });
        button2 = new JButton("Random");
        add(button2);
        button2.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent e) {
                setBoard(randomBoard());
            }
        });
        button3 = new JButton("Clear");
        add(button3);
        button3.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent e) {
                setBoard(clearBoard());
            }
        });
        addMouseListener(new MouseListener() {
            @Override
            public void mouseClicked(MouseEvent e) {
            }

            @Override
            public void mouseEntered(MouseEvent e) {
            }

            @Override
            public void mouseExited(MouseEvent e) {         
            }

            @Override
            public void mousePressed(MouseEvent e) {
                getBoard()[e.getY() / multiplier][e.getX() / multiplier] = !getBoard()[e.getY() / multiplier][e.getX() / multiplier];
            }

            @Override
            public void mouseReleased(MouseEvent e) {       
            }
        });
        timer.start();
    }

    public int getMultiplier() {
        return multiplier;
    }

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

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

    @Override
    public void paintComponent(Graphics g) {
        super.paintComponent(g);
        for (int i = 0; i < height; i++) {
            for (int j = 0; j < width; j++) {
                g.setColor(board[i][j] ? Color.green : Color.gray);
                g.fillRect(j * multiplier, i * multiplier, multiplier - 1, multiplier - 1);
            }
        }
        if (isActive) {
            timer.start();
        }
        else {
            timer.stop();
            repaint();
        }
    }

    public void actionPerformed(ActionEvent e) {
        board = nextGeneration();
        repaint();
    }

    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 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 boolean[][] 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;
                }
            }
        }
        return nextBoard;
    }   
}

and this GameOfLife class:

import javax.swing.JFrame;

class GameOfLife {
    public static void main(String[] args) {
        Board boardPanel = new Board();
        JFrame frame1 = new JFrame();
        frame1.setTitle("The Game of Life");
        frame1.setSize(boardPanel.getBoard()[0].length * boardPanel.getMultiplier() + 5, boardPanel.getBoard().length * boardPanel.getMultiplier() + 27);
        frame1.setResizable(false);
        frame1.setVisible(true);
        frame1.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame1.add(boardPanel);
    }
}

As you can tell, it's meant to represent John Conway's Game of Life on a JFrame. It's my first project using animation and graphics, so I expect there to be a few bad practices. Could anyone provide an honest, critical code review?

Thanks

\$\endgroup\$
3
  • \$\begingroup\$ Give better names for button1, frame1, etc. \$\endgroup\$ May 15 '13 at 9:06
  • \$\begingroup\$ Since the following suggestions are too obvious to deserve an answer, I'll tell them in this comment: replace boolean[][] emptyBoard = {{.... with boolean[][] emptyBoard = new boolean[10][10] replace new MouseListener with new MouseAdapter and remove empty methods. A just as obvious but not as trivial suggestion is to separate the logic from the swing dependent code. \$\endgroup\$ May 15 '13 at 13:22
  • \$\begingroup\$ Some suggestions on how to separate view and model: Move things elsewhere until you do not import anything from java.awt or javax.swing. If you were following single responsibility principle closely the following should be easy tasks: - start two instances of the game in two frames in the same process (from the same main() method) - add a freeze button to enable the grid to evolve but do not update the display and an unfreeze to restart the updates to the display \$\endgroup\$ May 16 '13 at 13:17
3
\$\begingroup\$
  1. As was suggested in the comments, initialize your matrix as boolean[][] emptyBoard = new boolean[10][10], as in Java booleans will default to false.
  2. Document your code: all IDEs will have some support for JavaDoc, use it.
  3. Also suggested in the comments, variables should have better names; especially GUI elements (you might not realize the importance of this because in your case you have few elements, but doing this in larger applications will slowly become a royal PITA).
  4. Separate logic from presentation. Don't do GUI operations and the game calculations in the same class (i.e. Board).
  5. You shouldn't do this:

    for (int[] i: surrounding) {
        try {
            if (board[i[0]][i[1]]) {
                count++;
            }
        }
        catch (ArrayIndexOutOfBoundsException e) {}
    }
    

    Properly defining surrounding should avoid any ArrayIndexOutOfBoundsException.

  6. This is mainly a personal preference, but what you have in main() (the GUI parts) I would put in one (or more) separate methods.
  7. Another personal preference is using ArratList instead of arrays.
\$\endgroup\$

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.