2
\$\begingroup\$

my Java needed a little refresher so I implemented the Game of Life as a little excercise. The game is working as expected, but I'm sure there is some room for improvement.

Here is the code (skipping the interfaces, packages and imports)

/**
 * Controller Implementation responsible for starting the game and keeping it
 * running.
 */
public class Controller implements IController {
    private IModel model;
    private final ScheduledExecutorService scheduler = Executors.newScheduledThreadPool(1);

    /**
     * Initialize a new controller and implicitly a model and view as well.
     * Start a loop updating the game.
     */
    public Controller() {
        this.model = new Model(50, 50, this);
        this.model.initializeModel();
    }

    @Override
    public void nextTick() {
        this.model.updateModel();
    }

    @Override
    public void randomSeed() {
        this.model.initializeModel();
    }

    @Override
    public void start() {
        final Runnable tickRunner = new Runnable() {
            @Override
            public void run() {
                nextTick();
            }
        };
        this.scheduler.scheduleAtFixedRate(tickRunner, 0, 50, TimeUnit.MILLISECONDS);
    }
}



/**
 * Model implementation with a simple array holding the game data. Responsible
 * for updating the view.
 */
public class Model implements IModel {
    public static int NUM_COLS;
    public static int NUM_ROWS;
    private final boolean[] currData;
    private final boolean[] nextData;
    private final Cursor cursor;
    private IView view;

    /**
     * Instantiate a new model
     * 
     * @param x
     *            number of rows of the game field.
     * @param y
     *            number of columns of the game field.
     * @param controller
     *            the controller to pass to the view.
     */
    public Model(final int x, final int y, final IController controller) {
        this.view = new SwingView(controller);
        NUM_COLS = x;
        NUM_ROWS = y;
        this.currData = new boolean[NUM_COLS * NUM_ROWS];
        this.nextData = new boolean[NUM_COLS * NUM_ROWS];
        this.cursor = new Cursor();
    }

    @Override
    public void initializeModel() {
        for (int i = 0; i < this.currData.length; i++) {
            this.currData[i] = Math.random() >= 0.5;
        }
        this.view.updateView(this.currData);
    }

    @Override
    public void updateModel() {
        getNewData();

        System.arraycopy(this.nextData, 0, this.currData, 0, this.currData.length);

        this.view.updateView(this.currData);
    }

    private void getNewData() {
        for (int i = 0; i < this.currData.length; i++) {
            updateCell(i, getNumberOfAliveNeighbours(i));
        }
    }

    private void updateCell(final int i, final int numberOfNeighbours) {
        final boolean isCellAlive = this.currData[i];

        if (isCellAlive) {
            if (numberOfNeighbours < 2 || numberOfNeighbours > 3) {
                this.nextData[i] = false;
            } else {
                this.nextData[i] = true;
            }
        } else {
            if (numberOfNeighbours == 3) {
                this.nextData[i] = true;
            }
        }
    }

    private int getNumberOfAliveNeighbours(final int i) {
        int aliveNeighbours = 0;

        this.cursor.setPosition(i);

        if (this.currData[this.cursor.moveRight()]) {
            ++aliveNeighbours;
        }

        if (this.currData[this.cursor.moveDown()]) {
            ++aliveNeighbours;
        }

        if (this.currData[this.cursor.moveLeft()]) {
            ++aliveNeighbours;
        }

        if (this.currData[this.cursor.moveLeft()]) {
            ++aliveNeighbours;
        }

        if (this.currData[this.cursor.moveUp()]) {
            ++aliveNeighbours;
        }

        if (this.currData[this.cursor.moveUp()]) {
            ++aliveNeighbours;
        }

        if (this.currData[this.cursor.moveRight()]) {
            ++aliveNeighbours;
        }

        if (this.currData[this.cursor.moveRight()]) {
            ++aliveNeighbours;
        }

        return aliveNeighbours;
    }

    private class Cursor {
        private int pos;
        private final int MAX_POS = (Model.NUM_ROWS * Model.NUM_COLS);

        public Cursor() {
            super();
        }

        public void setPosition(final int pos) {
            this.pos = pos;
        }

        public int moveRight() {
            if (this.pos >= this.MAX_POS - 1) {
                this.pos = -1;
            }
            return ++this.pos;
        }

        public int moveLeft() {
            if (this.pos <= 1) {
                this.pos = this.MAX_POS;
            }
            return --this.pos;
        }

        public int moveDown() {
            this.pos += Model.NUM_ROWS;
            if (this.pos > this.MAX_POS - 1) {
                this.pos -= this.MAX_POS;
            }

            return this.pos;
        }

        public int moveUp() {
            this.pos -= Model.NUM_ROWS;
            if (this.pos < 0) {
                this.pos += this.MAX_POS;
            }
            return this.pos;
        }
    }
}


/**
 * View implementation using Swing for the GUI.
 */
public class SwingView extends JFrame implements IView {
    /**
     * Auto generated by Eclipse.
     */
    private static final long serialVersionUID = -1538005133913482652L;
    private final JButton btnRnd = new JButton("(Re)Start");
    private Rectangles rectangles;
    final IController controller;

    /**
     * Initialize the Swing view and create controls.
     * 
     * @param controller
     *            the controller which is triggered by controls.
     */
    public SwingView(final IController controller) {
        super();
        this.setTitle("Game of Life");
        this.getContentPane().setLayout(null);
        this.setBounds(100, 100, 625, 575);
        addControls();
        this.setVisible(true);
        this.setDefaultCloseOperation(EXIT_ON_CLOSE);
        this.controller = controller;
    }

    private void addControls() {
        this.btnRnd.setBounds(500, 0, 100, 30);
        this.btnRnd.addActionListener(new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e) {
                SwingView.this.controller.randomSeed();
            }
        });
        this.add(this.btnRnd);
    }

    @Override
    public void updateView(final boolean[] data) {
        if (this.rectangles == null) {
            this.add(new Rectangles(data));
        } else {
            this.rectangles.setNewData(data);
        }
        this.revalidate();
        this.repaint();
    }

    private class Rectangles extends JPanel {
        /**
         * Auto generated by Eclipse.
         */
        private static final long serialVersionUID = -8352502831091810753L;
        private boolean[] data;
        private static final int RECT_SIZE = 10;

        public Rectangles(final boolean[] data) {
            this.data = data;
            this.setBounds(0, 0, data.length * RECT_SIZE, data.length * RECT_SIZE);
        }

        public void setNewData(final boolean[] data) {
            this.data = data;
        }

        @Override
        protected void paintComponent(final Graphics g) {
            super.paintComponent(g);

            for (int i = 0; i < this.data.length; i++) {
                if (this.data[i]) {
                    g.setColor(Color.BLACK);
                } else {
                    g.setColor(Color.WHITE);
                }
                g.fillRect((i % Model.NUM_ROWS) * RECT_SIZE, (i / Model.NUM_COLS) * RECT_SIZE, RECT_SIZE, RECT_SIZE);
            }
        }
    }
}

The code is also available here: https://github.com/totoMauz/gameOfLife

My one thouhts and remarks:

I started with a multi-dimensional array but it seemed a little sluggish after a while so I switched to a normal one, this helped a litte.

The MVC pattern seems a little bit forced, but I did it intentionally because it biggest advantage is that you theoretically can switch an implementation. If have yet to see that in practive though...I'm also not sure how to correctly instantiate the components to get the controller to the view.

The field is randomized, I do not intent to extend it in a way that the player can mark starting living positions.

The memory consumption is rather high (~90 MB) and the performance seems to degenerate over time, but I'm not seeing anything obvious (haven't profiled yet)

Thanks :)

\$\endgroup\$
5
\$\begingroup\$
    if (isCellAlive) {
        if (numberOfNeighbours < 2 || numberOfNeighbours > 3) {
            this.nextData[i] = false;
        } else {
            this.nextData[i] = true;
        }
    } else {
        if (numberOfNeighbours == 3) {
            this.nextData[i] = true;
        }
    }

You should get in the habit of doing the following:

If an if statement has a body of setting something to a boolean, and an else body of setting to the negated value of that boolean (e.g. if (x) { y = true; } else { y = false; }) then don't bother with the if statement and directly assign the value.

If an else statement only contains an if or an if-else(if-else) chain, pull the inner chain up a level in scope.

Specifically:

    if (isCellAlive) {
        this.nextData[i] = !(numberOfNeighbours < 2 || numberOfNeighbours > 3);
    } else {
        if (numberOfNeighbours == 3) {
            this.nextData[i] = true;
        }
    }

This is what the code looks like after you apply the first rule...

    if (isCellAlive) {
        this.nextData[i] = !(numberOfNeighbours < 2 || numberOfNeighbours > 3);
    } else if (numberOfNeighbours == 3) {
        this.nextData[i] = true;
    }

And this is what it looks like after applying the second rule.

You get much shorter code.

Though, I don't think it's that good yet... I don't like the negation here:

this.nextData[i] = !(numberOfNeighbours < 2 || numberOfNeighbours > 3);

We can invert the condition to get rid of the negation:

this.nextData[i] = numberOfNeighbours >= 2 && numberOfNeighbours <= 3;

But, this seems silly for something that is essentially "has 2 or 3 neighbors". If we make it more explicit...

this.nextData[i] = numberOfNeighbours == 2 || numberOfNeighbours == 3;

This seems better to me because it writes the requirement "a cell stays alive it if it has 2 or 3 neighbors" much more clearly.

\$\endgroup\$
4
\$\begingroup\$

It looks pretty straight forward, the intention is mostly quite clear and the code is clean.

Some suggestions about code style (aka personal preference):

  • this: I try to avoid using this, it's usually not necessary.
  • I-Prefix for interfaces. I usually go with Interface and InterfaceImpl, I think the I-prefix is a c# thing? :P; I usually think in interfaces and do not care about its implementation,...
  • Auto-generated serial thingy: I usually disable the warning in eclipse and only generate them if necessary.
  • NUM_COLS / currData / ...: I'd go with NUMBER_OF_COLUMNS / currentData. In SwingView you got "btnRnd", the intention is not quite clear here, what the button should do, rnd can mean round as in game round, or as in rounding, or maybe random. To really know, you have to read more code, that's why I try to avoid abbreviations.
  • final boolean isCellAlive I had a discussion on stackoverflow, one was asking why people do not use final when a variable will not be changed within a method (https://stackoverflow.com/questions/43342452/why-does-java-util-arrays-have-so-little-final-usage#comment73753514_43342452) - I usually only use it when it's necessary, other people use it always.
  • ++var vs var++. I usually only see ++var when I teach it to our apprentices.

Other things (no special ordering, sorry for jumping around :-)

  • Controller: You pass 50,50 to the Model, it's not clear what those 50,50 mean, maybe represent it as another var/constant, to make that line clear
  • Cursor: I thought about mouse cursor first, Cursor in general can have too many meanings, maybe use an adjective to make it more self explanatory.
  • Model constructor: You pass x and y, and assign it to static int NUM_COLS and NUM_ROWS. Usuall x and y are understood as position/coordinates, not as "maxRows", they should be renamed. Also: NUM_COLS/NUM_ROWS are static, they should be instance variable as well and must not be assigned during the creation of an object and you should change the access from the other components to get that information. Also, about the JavaDoc: If you rename for example x to numberOfRows, the documentation is obsolete. (imo 99% of comments are not necessary)
  • Model: this.view = new SwingView -> Shouldn't it be possible to inject the View-interface, not the implementation? Either inject via constructor or post-construct-setter.
  • Model: Model has a dependency to View and Controller, it can have a dependency to view, but it must not have a dependency to the controller. Bidirectional dependencies should be avoided in general, it couples components together.
  • Model.updateModel(): I don't understand why you copy the data into another array?
  • Model.getNewData(): The method updates cells (calls updateCell), that method should be renamed. The get-prefix tells the developer, that he will receive something (other than void).
  • Model.updateCell(): I can't remember the game concept - I do understand what the method does, but I don't understand why. Maybe it's a good idea to make the intention of this method more clear or maybe it's also the case that one must just know
  • Model.getNumberOfAliveNeighbours: yeaaah... maybe split that up somehow? Maybe write getPossibleCursors() and loop through those?
  • SwingView.addControls(): I don't get it why randomSeed must be called when the button is clicked?
  • data - data is usually a very dubious term, because it can mean anything. At least I wouldn't expect that it would be a boolean array. I think this should be renamed.
  • Rectanges: mmmh not sure about that: You pass the data within the constructor and at the same time you provide a setNewData method. Maybe it's better to load the data directly from the model. Or change the updateView method to "when rectangles is null, create a new one" and remove the else block afterwards and call the setNewData in both cases.
  • Rectangles: RECT_SIZE: ... should be RECTANGLE_WIDTH, I suppose?
  • Rectangles: Why you no foreach-loop?
  • Rectangles: I would have created a 'paintable' Rectangle with one data/boolean as constructor. And have the Frame have a List of Rectangles, or wrap Rectangles into a separate type, it's usually better to use .. ehrm. 'singular' types.

MVC in general:

  • Yes, you should be able to switch the implementation, but not sure if you can do it that easily with your approach (beside the bidirectional dependency). The MVC pattern is considered a presentation pattern, your game logic is within the model, though - I'm not sure if other people are wrong, or if I just have another mindset about that... Think about the following: If you want to test your game logic, you will always instantiate Model with a IController, within the constructor, the View will be instantiated. In my opinion, the game logic itself should be decoupled from the presentation layer completely, so if you want to change your design pattern in your presentation layer, your isolated game logic will stay the same. Also consider: If it's a client-server architecture, example http, will it still work? HTTP is maybe a bit of a cruel example, but at least consider what you would have to do, if you want to provide a console client, that's usually a good approach to verify your design.
  • about memory consumption: You should read a bit about gc in java, the memory consumption can be totally fine, because: A java process starts with the default min and max memory settings and will grow to the max memory. If the available memory gets tight, it will collect (that was very abstract)

Hope that helps,...

\$\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.