Another conways game of life

[Gerard Charles]

Following up on thread https://coderanch.com/t/40/700311/code-reviews/engineering/version-Conway-Game-Life, I've implemented an alternate version, mostly because I find it easier to express my thoughts in code rather than prose. The
problem is stated at https://en.wikipedia.org/wiki/Conway%27s_Game_of_Life#Rules

https://github.com/Gerardwx/conway/

There's a console main in Demo.java, and a JavaFX based on in GraphicDemo.java .

[Stephan van Hulst]

I'll tell you the same thing I've told Liutauras. If you create your work on separate branches and then make pull requests when you want your code to be reviewed, it's much easier to review and keep the comments in one place.

Let us know what commit on master you want to use as a baseline for an initial review.

[Gerard Charles]

Stephan van Hulst wrote:I'll tell you the same thing I've told Liutauras. If you create your work on separate branches and then make pull requests when you want your code to be reviewed, it's much easier to review and keep the comments in one place.

Let us know what commit on master you want to use as a baseline for an initial review.

Good idea. I created a branch at  https://github.com/Gerardwx/conway/tree/review for the review.

[Liutauras Vilda]

But that way it feels that Ranch forums will miss benefits as most of the discussion will be taken offline.

[salvin francis]

Nitpick ...

World.java --> setNeighbors

/**
     * tell a cell who its neighbors are
     * @param x
     * @param y
     */
    private void setNeighbors(int x, int y) {
        int offsets[][] = {
                {-1, -1},
                {-1, 0},
                {-1, 1},
                {0, -1},
                {0, 1},
                {1, -1},
                {1, 0},
                {1, 1},
        };
        ArrayList<Cell> neighbors = new ArrayList(offsets.length);
        for (int i = 0; i < offsets.length; i++) {
            int xypair[] = offsets[i];
            int nx = adjacent(x, xypair[0], width);
            int ny = adjacent(y, xypair[1], height);
            neighbors.add(cells[nx][ny]);
        }

        cells[x][y].setNeighbors(neighbors);
    }

Offsets are always going to be the same for square block systems (unless you're planning for a hexagonal system or similar) . Maybe it should be a static final variable ? I know that you are just calling that method in the constructor iteration. But, each iteration will create a new array.

I am looking forward to run your code on my machine and see it in action !! Will update you soon.

[Knute Snortum]

Another nitpic:  

   ArrayList<Cell> neighbors = new ArrayList<>(offsets.length);

Without the underlined part you get a compiler warning.

[Liutauras Vilda]

I'm not going to question design in general, I consider it just as one of the possible designs. Certainly there are many, some are more flexible, some are less, some are more concise, some less. So will go through what you have  for now.

1.
Cell field states[]
For me this variable name is misleading. It really represent not just states, but current and next generation's state, for me that is subtle, but very important distinction.

2. Cell.setNeighbors(Collection<Cell> neighbors)
No parameter validation.

3.

public void setAlive() {
    assert Clock.getInstance().getCycle() == 0;
    states[0] = true;
}

For me such method gives an impression that Clock has no clear use. So once assert passes, use clock's cycle to access index, not hard code it.

4. Cell.setDead() same as in point 3.

5. ClassicCell.compute() very ciphered implementation. Without knowing rules, would have hard times deciphering cell lifecycle's idea.
5.1 Would avoid fall-through.

6. ClassicCell. Why it is public and non final? Same as with most of other classes.

7. ClassicCell. Poor javadoc. No return type. Formatting issues.

8. World constructor. Inconsistent use of 'this', cell assignment is missing that one.

9. World constructor. Allows create world in inconsistent state. i.e.: negative width and height. Actually that would result in crashing system. Also missing function parameter validation which could result in NPE.

10. Knowledge duplication by passing in x, y as coordinates. Why not to have wrapped in a class? If new coordinate would appear, you'd need to re-design whole system instead of just potential class.

11. World.get(), World.setAlive() no parameters validation. Can result in ArrayIndexOutOfBoundsException.

12. World.setNeighbors, World.adjacent(). Too complex personally for me. Poor variable names, i.e.: nx, ny, xypar,  Wouldn't be obvious from maintenance point of view. No parameters check.

13. ArrayList<Cell> neighbors = ... Keep List<Cell> neighbors = ... as implementing class may change in a future, would be less hassle to update.


----
Does this system behave in an expected manner? How you verified that? If you'd refactor system to a better, how would you verify it still behaves in an expected manner?

[Liutauras Vilda]

salvin francis wrote:Offsets are always going to be the same for square block systems (unless you're planning for a hexagonal system or similar) . Maybe it should be a static final variable ?

That would look dangling in a World class with no clear indication why is it for. I'll call it as Junilu, a smell, just don't know what kind of. Maybe misassigned responsibility?

[Gerard Charles]

salvin francis wrote:Nitpick ...

World.java --> setNeighbors

/**
     * tell a cell who its neighbors are
     * @param x
     * @param y
     */
    private void setNeighbors(int x, int y) {
        int offsets[][] = {
                {-1, -1},
                {-1, 0},
                {-1, 1},
                {0, -1},
                {0, 1},
                {1, -1},
                {1, 0},
                {1, 1},
        };
        ArrayList<Cell> neighbors = new ArrayList(offsets.length);
        for (int i = 0; i < offsets.length; i++) {
            int xypair[] = offsets[i];
            int nx = adjacent(x, xypair[0], width);
            int ny = adjacent(y, xypair[1], height);
            neighbors.add(cells[nx][ny]);
        }

        cells[x][y].setNeighbors(neighbors);
    }

Offsets are always going to be the same for square block systems (unless you're planning for a hexagonal system or similar) . Maybe it should be a static final variable ? I know that you are just calling that method in the constructor iteration. But, each iteration will create a new array.

I am looking forward to run your code on my machine and see it in action !! Will update you soon.

That's a good point. I find myself wondering whether a JVM could / would optimize the initialization somehow (see https://shipilev.net/jvm-anatomy-park/6-new-object-stages/ for some interesting discussion of what the JVM can /can't do). But more importantly your post suggests an optimization path and an opportunity for making the design more extendable. I changed setNeighbors to take the offsets as a parameter (and incorporated other good suggestions in the thread; declare neighbors as List instead of ArrayList and adding the missing <>)

private void setNeighbors(int x, int y, int offsets[][]) {
        List<Cell> neighbors = new ArrayList<>(offsets.length);

and then changed the World constructor to protected, with an offsets parameter:

protected World(int width, int height, BiFunction<Integer, Integer, Cell> creator, int offsets[][]) {
        this.width = width;
        this.height = height;
        cells = new Cell[width][height];
        iterate((x, y) -> cells[x][y] = creator.apply(x, y));
        iterate((x, y) -> setNeighbors(x, y, offsets));
    }

I then added subclass ClassicWorld as shown, using a static method to create the block array one time:

public class ClassicWorld extends World {

    /**
     * @param width: cells across
     * @param height: cells down
     * @param creator: specify type of cell to create
     */
    public ClassicWorld(int width, int height, BiFunction<Integer,Integer,Cell> creator) {
        super(width, height, creator, classicOffsets());
    }

    private static int[][] classicOffsets() {
        int offsets[][] = {
                {-1, -1},
                {-1, 0},
                {-1, 1},
                {0, -1},
                {0, 1},
                {1, -1},
                {1, 0},
                {1, 1},
        };
        return offsets;
    }

}

[Gerard Charles]

Liutauras Vilda wrote:I'm not going to question design in general, I consider it just as one of the possible designs. Certainly there are many, some are more flexible, some are less, some are more concise, some less. So will go through what you have  for now.

Replies interpersed below
1.
Cell field states[]
For me this variable name is misleading. It really represent not just states, but current and next generation's state, for me that is subtle, but very important distinction.
* Changed it to generations
2. Cell.setNeighbors(Collection<Cell> neighbors)
No parameter validation.
* What would this look like?

3.

public void setAlive() {
    assert Clock.getInstance().getCycle() == 0;
    states[0] = true;
}

For me such method gives an impression that Clock has no clear use. So once assert passes, use clock's cycle to access index, not hard code it.
* Good idea -- in fact, adding an explicit method to Clock can make the intent clearer. Revised code:

public void setAlive() {
        assert Clock.getInstance().isBeginningOfSimulation();
        generations[Clock.getInstance().getCycle()] = true;
    }

5. ClassicCell.compute() very ciphered implementation. Without knowing rules, would have hard times deciphering cell lifecycle's idea.
5.1 Would avoid fall-through.
* Good thing the rules are posted as a comment right above then? Personally, I'm happy with the full functionality of the switch statement, including fall-throughs where they make sense, but I understand opinions vary.
6. ClassicCell. Why it is public and non final? Same as with most of other classes.
* For me, that's going to be the default implementation unless there's a compelling reason to make it otherwise.


8. World constructor. Inconsistent use of 'this', cell assignment is missing that one.
* "this" is required when a parameter (or local variable) of the same name is present. It's not needed for cell. (I understand some programmers don't like C++/Java's implicit this but I'm fine with it.)

9. World constructor. Allows create world in inconsistent state. i.e.: negative width and height. Actually that would result in crashing system. Also missing function parameter validation which could result in NPE.
* What alternative would you suggest?

10. Knowledge duplication by passing in x, y as coordinates. Why not to have wrapped in a class? If new coordinate would appear, you'd need to re-design whole system instead of just potential class.
* I'm going to ponder that one.

12. World.setNeighbors, World.adjacent(). Too complex personally for me. Poor variable names, i.e.: nx, ny, xypar,  Wouldn't be obvious from maintenance point of view. No parameters check.
* nx is scratch variable name with a scope of three lines. A rose by any other name...

13. ArrayList<Cell> neighbors = ... Keep List<Cell> neighbors = ... as implementing class may change in a future, would be less hassle to update.
* Good point, fixed.

I've pushed  new version with suggested change to the master branch: https://github.com/Gerardwx/conway/tree/master/conway

[Liutauras Vilda]

Gerard Charles wrote:2. Cell.setNeighbors(Collection<Cell> neighbors)
No parameter validation.
* What would this look like?

this.neighbors = Objects.requireNonNull(neighbors);

But that solves little. Whole problem with such setters are that you rely on the API user, whether the user won't forget to set neighbors, otherwise your system would crash because of NPE. It lets you create a Cell in an inconsistent state. Why not to insist that in constructor?

Gerard Charles wrote:5. ClassicCell.compute() very ciphered implementation. Without knowing rules, would have hard times deciphering cell lifecycle's idea.
5.1 Would avoid fall-through.
* Good thing the rules are posted as a comment right above then?

As long as you maintain them. But I personally like to understand the logic from code and not the comments. No return type in javadoc however.

Gerard Charles wrote:8. World constructor. Inconsistent use of 'this', cell assignment is missing that one.
* "this" is required when a parameter (or local variable) of the same name is present. It's not needed

In such case your explanation contradicts with the facts in Cell class constructor. Personally myself in constructor always use 'this', and in some other places where it improves readability. But that is a programmer's choice, not insisted by Java in these cases.

Gerard Charles wrote:9. World constructor. Allows create world in inconsistent state. i.e.: negative width and height. Actually that would result in crashing system. Also missing function parameter validation which could result in NPE.
* What alternative would you suggest?

One of alternatives is to validate them.

[salvin francis]

Liutauras Vilda wrote:..That would look dangling in a World class with no clear indication why is it for. ... Maybe misassigned responsibility?

This does not smell bad or look dangling to me :

public class World {
  private static final int CELL_NEIGHBOR_OFFSETS [][] = {{...},{...}}
  ...
}

But.. I still like Gerard's implementation better for now of moving it to ClassicWorld into a method instead of a constant.

[Liutauras Vilda]

Gerard Charles wrote:

iterate((x, y) -> cells[x][y] = creator.apply(x, y));

I think it is misleading to use x and y names with an array of arrays. x really represents rows within an array, which kind of go along y-axis, while y uses x-axis idea.

[Junilu Lacar]

I have a bunch of questions about the implementation but the first and biggest one is: Where are the unit tests?!!

Sans tests, I would have to go to the main class and start digging from there.

This line in Demo.java does not speak to me clearly.
       World w = new ClassicWorld(10,10,(x,y) -> new ClassicCell());
It's not clear nor intuitive why I would need to pass a lambda expression like that to World. From the outside looking in, this smells like inappropriate intimate knowledge of the internal machinations of the World class, especially since there are no tests that tell me a story of the design.

If anything, this is what I might expect to see:

World gameOfLife = new ClassicWorld(10,10, ClassicCell::new);

I would read this as: Create a ClassicWorld that is 10x10 in dimension and populate it with ClassicCell instances.

[Junilu Lacar]

More questions about design choices that I don't understand:

Why does a cell need to keep a reference to its neighbors? What are you achieving by doing this? When calculating the neighbors, you're accessing World.cells anyway so what's the value/point of this extra complexity? Is it so that the cell can query its own neighbors? If so, I don't think the extra complexity is worth it.

I would just have the World class take on more responsibility for determining the neighbors for a particular cell.

[Junilu Lacar]

I don't get why there's a need to design the World class for inheritance. I only see one subclass of it and the only thing that it seems to specialize is the offsets.

These are design choices being made that don't have any apparent forces behind them. Why would we need to define a super-subclass relationship between World and ClassicWorld if the only thing that ClassicWorld provides are the offsets for the neighboring cells? Again, I don't see the value of introducing this kind of complexity this early on without any compelling forces that push the design in that direction.

Seems like a lot of speculative design going on here.

[Junilu Lacar]

I analyzed the relationship between World and ClassicWorld again and it gives off a very strong smell of inappropriate intimate knowledge.

Why does the ClassicWorld constructor need to pass neighbor offsets back up to the World constructor? This implies that ClassicWorld knows about the configuration of the World grid and that it knows it's a two-dimensional surface. This is very inappropriate intimate knowledge. In fact, any subclass of World will have to pass the exact same offsets back up to the World constructor. That makes the smell infinitely clear.

[Junilu Lacar]

I get where the idea of the Clock class comes from but to me, it feels like it's going far with trying to be OO. Again, this is a responsibility that could probably be put back in the World class itself. All those asserts in the Cell class around what the state of the Clock should be pollutes and confuses the intent of the code. It's unintuitive to me why you would need to assert something about Clock when you're setting a Cell alive or dead. Similarly, the use of the Clock is unintuitive in the calculateNext() method. All the related ideas are just spread out too much and I have to bounce from one class to another just to understand how everything fits together. This is not the ideal way to write OO code.

To me, it breaks cognitive encapsulation. That is, I can't just look at one thing and get a coherent idea of how things work. I have to look at multiple things and piece all the different ideas together myself. This makes the code very difficult to understand.

[Junilu Lacar]

One positive note about this version of GoL: It gives me an inkling of an idea of how to implement it such that a World can have a shape other than just a rectangular, two-dimensional area. For example, I might have code like this:

   World threePointedWorld = new TriangularWorld(base, height);
   World eggWorld = new EllipticalWorld(base, height);
   World mmaWorld = new OctagonalWorld(breadth);
   World globe = new SphericalWorld(radius);

[Junilu Lacar]

That idea for SphericalWorld would be very interesting, especially if you say that it is entirely made up of cells, not just the surface but also everything underneath the surface of the sphere as well.

You could get into all kinds of complex and dynamic changes here. So, under the surface, you have to imagine a three-dimensional space. If you then represent each cell and its neighbors as a cube, then each cell under the surface of the sphere would have up to twenty-six neighbors (nine in the layer above, eight in the layer around it, and another nine in the layer below), kind of like what the core of a standard 3x3 Rubik's cube would have.

A cell on the surface, however, would only have up to seventeen neighbors since it has no layer of neighbors above it, only eight around it and nine below the surface.

Depending on the transition rules you define, that could potentially lead to very cool representations of erosion, eruption, and movement of surface features, as if by geological forces.

Wow, that would be pretty challenging to render as a 3-D image. Seems like it could be a whole semester project at least. Maybe even a master's thesis.

[salvin francis]

Found this 3d shape on wiki

[Junilu Lacar]

I'm going to walk through the code to explain in detail why I think the World class is very smelly.

In Demo.java, we see
World w = new ClassicWorld(10, 10, (x,y) -> new ClassicCell());
I have already expressed my objection to this code before but let me reiterate: The semantics of the lambda is unclear. It smells of inappropriate knowledge of intimate details of the implementation of World. I get the first two parameters, they define the boundaries of the new World object. But WTH is this lambda expression trying to say? It looks like it is returning a new ClassicCell() instance but what's with the (x, y) parameters? And why aren't they even used? This is a red flag to me that the design is not coherent. There should be no elements in the design that are not used.

[Junilu Lacar]

Next, we look at the ClassicWorld class:

   /**     * @param width: cells across     * @param height: cells down     * @param creator: specify type of cell to create     */    public ClassicWorld(int width, int height, BiFunction<Integer,Integer,Cell> creator) {        super(width, height, creator, classicOffsets());    }    private static int[][] classicOffsets() {        int offsets[][] = {                {-1, -1},                {-1, 0},                ...
Now we see that the constructor is just a passthrough with additional baggage of the offsets array.

Here's another suspiciously smelly piece: the last parameter of the constructor is defined as BiFunction<Integer, Integer, Cell> creator)

Again, the semantics of a "creator" function that takes two integers and returns a cell seem too intimate with details about the configuration of the World. But we can reserve judgement until we see the super constructor. Then my suspicions are confirmed:

In World.java:
   /**     * @param width:   cells across     * @param height:  cells down     * @param creator: specify type of cell to create     */    protected World(int width, int height, BiFunction<Integer, Integer, Cell> creator, int offsets[][]) {        this.width = width;        this.height = height;        cells = new Cell[width][height];        iterate((x, y) -> cells[x][y] = creator.apply(x, y));        iterate((x, y) -> setNeighbors(x, y, offsets));    }
I now smell the strong odor of unnecessary complexity on top of the inappropriate intimate knowledge of internals.

Why do width and height need to be kept as instance variables? They are not needed. Why? Because having them is a violation of DRY! The cells array already holds that information when you create it on line 21!

How do you get width?

public int getWidth() {
    return cells.length;
}

How do you get height?

public int getHeight() {
    return cells[0].length;
}

[Junilu Lacar]

Let's look at the constructor again:
   /**     * @param width: cells across     * @param height: cells down     * @param creator: specify type of cell to create     */    public ClassicWorld(int width, int height, BiFunction<Integer,Integer,Cell> creator) {        super(width, height, creator, classicOffsets());    }
Line 11 in the JavaDocs indicates to me a lack of clarity of purpose and intent. This comment lies or misleads.

How does a BiFunction<Integer, Integer, Cell> "specify type of cell to create"?

It would be more understandable if the declaration were:
    * @param cellCreator: a Supplier of Cell instances, used to populate this world     */    public ClassicWorld(int width, int height, Supplier<Cell> cellCreator) {        super(width, height, cellCreator, ...);    }
This leads to the constructor call I suggested earlier:

   World gameOfLife = new ClassicWorld(10, 10, ClassicCell::new);

[Junilu Lacar]

I will leave it to you guys to smell out the rest of the World constructor, particular the parts where iterate is being called.

Let me give you a hint though. Consider this code and what the JavaDocs declares it does:
  /**     * call method on each cell, sequentially     *     * @param consumer     */    public void iterate(CellConsumer consumer) {        for (int y = 0; y < height; y++) {            for (int x = 0; x < width; x++) {                consumer.apply(x, y);            }        }    }
How is someone supposed to see how a Cell method is being called on each cell here? Where is the reference to the Cell whose method is invoked? Where is the reference to the method being called? If you can't tell me where those are in this snippet of code, then that means this code is blatantly displaying an inappropriate intimate knowledge of implementation details that are elsewhere in the code.

[Gerard Charles]

Junilu Lacar wrote:I will leave it to you guys to smell out the rest of the World constructor, particular the parts where iterate is being called.

Let me give you a hint though. Consider this code and what the JavaDocs declares it does:
  /**     * call method on each cell, sequentially     *     * @param consumer     */    public void iterate(CellConsumer consumer) {        for (int y = 0; y < height; y++) {            for (int x = 0; x < width; x++) {                consumer.apply(x, y);            }        }    }
How is someone supposed to see how a Cell method is being called on each cell here? Where is the reference to the Cell whose method is invoked? Where is the reference to the method being called? If you can't tell me where those are in this snippet of code, then that means this code is blatantly displaying an inappropriate intimate knowledge of implementation details that are elsewhere in the code.

Oh, that's obviously wrong. Updated master:

 /**
     * iterate World coordinates, sequentially
     *
     * @param consumer
     */
    public void iterate(WorldConsumer consumer) {
        for (int y = 0; y < height; y++) {
            for (int x = 0; x < width; x++) {
                consumer.apply(x, y);
            }
        }
    }

[Junilu Lacar]

Names like WorldConsumer and CellConsumer are smelly.

Why not Consumer<World> or Consumer<Cell> instead?

See the difference?

[Gerard Charles]

Junilu Lacar wrote:Names like WorldConsumer and CellConsumer are smelly.

Why not Consumer<World> or Consumer<Cell> instead?

See the difference?

Consumer is already defined in the JDK. Generics don't work with primitives.

What's the definition of smelly  in the context of code?

[Piet Souris]

I had a quick first glance at the code. My impression was that the World class is dictating far too much, leaving only a marginal role for its subclasses (deliver the neighbors for the offsets). I have the feeling some World methods could be made abstract, to leave them for the subclasses. As said, this is my first impression,

Then, something I do not quite understand:

Junilu Lacar wrote:I'm going to walk through the code to explain in detail why I think the World class is very smelly.

In Demo.java, we see
World w = new ClassicWorld(10, 10, (x,y) -> new ClassicCell());
I have already expressed my objection to this code before but let me reiterate: The semantics of the lambda is unclear. It smells of inappropriate knowledge of intimate details of the implementation of World. I get the first two parameters, they define the boundaries of the new World object. But WTH is this lambda expression trying to say? It looks like it is returning a new ClassicCell() instance but what's with the (x, y) parameters? And why aren't they even used? This is a red flag to me that the design is not coherent. There should be no elements in the design that are not used.

First of all: it is clear that the World consists of a rectangular array, with coordinates x and y. The objection is that these are not used in the right part of the lambda.
Why is that a code smell? You could also have this lamda:

(x, y) -> (x % 2 == y % 2) ? new ClassicCell() : y == 0 : new BorderCell() : new ContemporaryCell(x, y);   // or similar

[Junilu Lacar]

Gerard Charles wrote:Generics don't work with primitives.

What primitives are you referring to?  Consumer<World> and Consumer<Cell> don't involve any primitives. Also, generics can actually work with primitives that can be autoboxed, like int. You can't declare Consumer<int> but you can certainly pass a primitive int to one because an int will get autoboxed to an Integer.

What's the definition of smelly  in the context of code?

In this context, unclear and non-intuitive semantics. The API seems to be designed with an underlying implementation already in mind. That's an implementer's perspective and the implementer knows way too much detail than the client. That is dangerous because it tends to create leaky abstractions, which are smells.

A better way to design is from the client's perspective, with only intent/purpose in mind. What would client code expect to know? To me, the only thing a client would reasonably know are the dimensions of the World they want to create. Even the previous suggestion I gave suffers from too much implementation knowledge it seems, where I suggested ClassicCell::new be passed instead.

As a client, I'm really only interested in creating a World that has cells in it. I don't think a client should care what particular "kind" of cells they may be. The World object should know what kind of Cell objects are compatible with it, so that knowledge should be encapsulated in World.

I think this should be enough to create a World:

World gameOfLife = new ClassicWorld(10, 10);   // a World configured as a rectangular grid

Specifying the kind of cell to be created just doesn't seem like a responsibility the client needs to take on. It's inappropriate to assign that responsibility to the client. If the client must supply a "creator" lambda like (x, y) -> new ClassicCell, then the client must know that the underlying implementation of the World() constructor calls this lambda with two parameters.

Piet Souris wrote:The objection is that these are not used in the right part of the lambda. Why is that a code smell?

Why ignore the values passed in to the lambda? It seems like a pointless exercise. Either there's a misunderstanding somewhere or the logic of the design is not coherent.

[Junilu Lacar]

Look at it this way: If I try to document this call with only intent/purpose in mind:

World gameOfLife = new ClassicWorld(10, 10, ClassicCell::new);

... 

/**
 * Represents a classic world with a two-dimensional rectangular field of play.
 */
public class ClassicWorld {

    /**
     * Creates a classic two-dimensional rectangular playing field.
     *
     * @param width the width of this world's playing field
     * @param height the height of this world's playing field
     * @param supplier the Cell Supplier used to populate the field
     *       with Cell objects.
     */
    public ClassicWorld(int width, int height, Supplier<Cell> supplier) { ... }

The documentation explains why the type is Supplier<Cell>: because it will be used to populate the field with Cell objects.

Now, look at the documentation of the constructor the way it is currently designed:

    /**
     * @param width:   cells across
     * @param height:  cells down
     * @param creator: specify type of cell to create
     */
    protected World(int width, int height, BiFunction<Integer, Integer, Cell> creator, int offsets[][]) {

That just is NOT coherent to me. It's not even complete. Look at the implementation code carefully. Where is a new cell assigned to cells[x][y]? That whole mechanism is convoluted and reminds me of a Rube Goldberg machine.

I can give you guys a cleaner version but I want you to think of a way to clean up this code so it's more straightforward and easier to follow. Right now, the logic is jumping through way too many hoops than it needs to.

[Junilu Lacar]

Also, width and height can be misleading when used with nested arrays.

int width = 3;
int height = 2;

int[][] misleading = new int[width][height];
===> [ [0, 0],
       [0, 0],
       [0, 0] ]    

int[][] better = new int[height][width];
===> [ [0, 0],
       [0, 0],
       [0, 0] ]    

// but even better:

int rows = 3;
int cols = 2;

int[][] evenbetter = new int[rows][cols];
===> [ [0, 0],
       [0, 0],
       [0, 0] ]             

[Ganesh Patekar]

Congratulations Gerard Charles,

Your question has made it to our Journal    

Have a Cow!