My version of Conway's Game of Life

[Liutauras Vilda]

Hello Ranchers,

For a long time I was fascinated about this project, but never attempted to implement it myself. That's changed.
So decided to create a personal construction site and work at it to practice and experiment with various design decisions implementing this cellular automaton.

For the past 3-4 days I wrote and re-wrote.. this project countless number of times (small portion of commits history recorded).
The version I have currently is something I think I could try to justify why I have it like that, and to what angle it has flexibility to offer so I chose to implement it that way.

However, do extra pairs of eyes see some illogical design decisions, or have tips what to think about how this could be improved further to avoid some potential limitations I could face when some hypothetical requirements change?

Don't get fooled by the width of the project (only very few source files), but it really taught me many lessons already, and I believe it will again.

From the very first versions I came up with, this recent version I think got shorter twice, I think it has more flexible design, it was way easier to test and achieve good enough test coverage. I'm currently at a stage when I need some subtle tips to move on further refinements (or.. fixes).

Please find project implementation at my GitHub area. For those who don't know the mentioned project, please read the description on Wikipedia.

Thanks.

[Jeanne Boyarsky]

In World, the indentation of this code misleads about what it does. Since the two forEach() statements are aligned, they imply that they are operating on the same stream.

IntStream.range(0, SIZE)
            .forEach(r -> IntStream.range(0, SIZE)
            .forEach(c -> map.put(Location.of(r, c), Cell.EMPTY)));

Which of course is impossible. I would recommend moving this to a helper message if you do it this way.

IntStream.range(0, SIZE)
            .forEach(c -> map.put(Location.of(r, c), Cell.EMPTY)))

More importantly, calling map.put() in a forEach isn't good practice. Is there any reason you can't use a collector which would dispense with the inner forEach() entirely.

[Jeanne Boyarsky]

Also, I'm not a fan of the default method in Mutate. It feels like a hack since it isn't used for legacy code.

Oh. And there are many good things too! But you asked for improvement suggestions so...

[salvin francis]

I ran that program and it just printed a huge log of dots and zeros !!!
Even a simple JFrame with a single textarea would be great.

Good job on that code, I liked it. I am glad to see that your glider "wraps-around" the world.

minor suggestions: Maybe World.SIZE could have an access modifier (probably public) ?

[Liutauras Vilda]

Jeanne Boyarsky wrote:In World, the indentation of this code misleads about what it does. Since the two forEach() statements are aligned, they imply that they are operating on the same stream.

First - thankful for a review and comments.

Agreed. Indentation was misleading. With streams kind of can get messy when always aligning by the dot ".". I simply now just let IDE to format for me based on by default pre-configured its rules.

Jeanne Boyarsky wrote:More importantly, calling map.put() in a forEach isn't good practice. Is there any reason you can't use a collector which would dispense with the inner forEach() entirely.

That's a good one! I was so concentrated on design, so got less than optimum at other points. Initially I simply had 2 habitual loops, so later decided to replace with forEach, hence got horrible blocks. Just refactored to use collectors you suggested in empty(); nextGeneration(); methods, indeed much better as it hides internals how data structure gets populated. However, a bit struggled until remembered how to do it with specific Map's implementation.

Jeanne Boyarsky wrote:I'm not a fan of the default method in Mutate. It feels like a hack since it isn't used for legacy code.

Well. I was researching about default before attempted to use it. To my disappointment, since Java moving towards modernising herself, I don't understand why default is so implied to be used only for legacy code. It is such a nice feature to have in my opinion (makes even slimmer gap between interface vs abstract class) and it fitted perfectly to my idea. Upon hypothetical requirements change mutable(s), i.e. Monster just need to implement that interface without providing any extras; and define their mutation rules. I'm not up (convinced yet) for removing it and implementing this method in the very same way in each and every concrete Mutable implementation.

Thanks for the comments once again!

[Liutauras Vilda]

salvin francis wrote:I ran that program and it just printed a huge log of dots and zeros !!!
Even a simple JFrame with a single textarea would be great.

Yet again, first of all, thanks for reviewing it.

Yeah. Agree, it isn't user friendly and much interactive yet. But I have created for myself an enhancement item, so at some point will get it done.

However, at this stage my main goal was to come up with a design so it would be simple to understand, easy enough extend and very importantly to test. Tried to keep separate pieces responsible as much as their need to be responsible for, so changing something I wouldn't need to rewrite tests from scratch and similar. Interesting, but initially that was exactly my experience. Started with semi TDD, thought came up with a fairly nice approach, however, trapped to a situation, that I was not able to test everything without exposing objects internals as for instance Location class was relying on World's internal data structure to count alive neighbours around, and I did want to have minimum getters, minimum mutation... etc. So most of the time such figuring out design, but I'll have time for GUI, just my designer's skills aren't anywhere great.

salvin francis wrote:I am glad to see that your glider "wraps-around" the world.

Yep, since all action happens on an infinite grid, I was looking for the way to put this concept in place.

Thanks for comments.

salvin francis wrote:minor suggestions: Maybe World.SIZE could have an access modifier (probably public) ?

Nah Stephan some time ago suggested, and once again re-read Effective Java by Joshua Bloch, so basically I give visibility as much as it is needed and no more. On that point of view, I'd prefer that even interfaces would provide an ability to implement package-private methods, as sometimes (and I faced this situation too), I exposed method (meaning had to make public) without real need of that, i.e. Cell.mutate() is public, while perfectly could work with package-private visibility.

[Stephan van Hulst]

I found that it's difficult in GitHub to keep track of comments unless they're all made on a pull request or a single commit.

When you have a stable-ish version of your code, can you make updates in separate branches and merge them to master with pull requests? That way, I can leave all my comments that aren't related to new code on a single commit on the master branch, and when you make a lot of changes in a short amount of time, a pull-request will nicely keep track of the conversation, both the changes and the comments.

[Liutauras Vilda]

In case somebody reads, one of my colleague here, Stephan, raised some concerns (behind the hood, within GitHub platform) regarding the topology, where I mixed up two different contexts Location and Direction into one - Location, and as a result represented directions where to move in order to find neighbours by reusing Location class (and that is bad), which actually represents an absolute path within a world map. So separated these lately and now there are Location and Direction (appear as an inner enum as it is needed just there for the moment).

Another point we discussed that I have an enum Cell which implements (/-ed actually) Mutable interface. And there he had a point. Enum is an immutable from Java's stand point, and to implement Mutable interface was a bit odd, even though the ideas underneath didn't clash much as simply two different domains interfered Java's (immutable enum) and automaton game (where cell mutates). As a result I renamed Mutable idea within the domain entirely, so it became:

Mutable -> Regenerable
Mutable.mutate() -> Regenerable.regenerate()
MutationRules -> RegenerationRules

And these make much more sense now. Thanks to him.

However, we still got an argument whether to represent Cell as an enum is a good idea. Maybe to have Cell as a regular class with an inner enum State ALIVE, ETC... would be better. I must say I had implemented that 2 days ago - well, more experiments to come.

It is very interesting little project to work on.

[Stephan van Hulst]

I probably should have put all of these in a single commit, but I have some more comments for you here:

8f1265b4823a29de9951ae0fc0b6ff6bcde157f3

f381544b2a9efc93fa5fec6d92af6cf8f42e20b7

5d43c650fd99014996a3eaf768959cb7bf88dada

[salvin francis]

I looked at your code a bit more in-depth today and the following questions came to my mind :

Why can one not instantiate World ? The only way you are allowing is via a empty() method which generates according to a fixed SIZE of 10

Why is World square ? (as in number of rows=number of columns)

I see that you have implemented this as a fixed world with cells which are either Alive or Empty. The way I approached this was that I made an implementation which only kept Alive cells. There were no "Empty" cells.
Actually this is what fits in the real world philosophy too. In real life, we would observe a 'petri dish' with some cells. These cells would grow/die depending on various conditions, the 'petri dish' is their domain/world. With the implementation of "Empty" it looks like the world has some "slots" where a cell can reside or not.

Just food for thought

[Piet Souris]

hi Liutauras,

I also had a look at your code. My first impression is that I get a sense of over-desinging here and there, making things more complex than necessay. Perhaps it is the modern way of dong, I don't know, but for instance this Builder.build() looks a bit cumbersome.

The use of a Map is understandable, permitting with ease an infinite world. But since you are limiting the row and columns to the size of the World, you might as well use a boolean[][] here.
The way you determine the neighbors of a Location is elegant, but again a bit cumbersome. But I wanted to remark this:
you have

private Location neighborOf(Location location) {
            return Location.of((location.rowIndex + rowIndex + World.SIZE) % World.SIZE,
                    (location.columnIndex + columnIndex + World.SIZE) % World.SIZE);
        }

but that method is not foolproof. To keep an index into the range (0, World.size (where is this World coming from?)), you should do something like:

int x = (location.rowIndex + rowIndex) % size;
return x < 0 ? x + size : x;

For instance, if you have Location(0, 0) and you invoke 'neighborOf(Location(-18, -18))' with a size of 10.

About the RegenerationRules, why do you fix the 2 and the 3, given that the class is final? I had a similar class, with two fields: List<Integer> stayAlive and ditto: becomeAlive, that were parameters of my constructor.

I admit: my remarks are a bit subjective, and my code does not use elegant Streams at all (it was made some years ago), time for me to do some updating. But please report your impressions at the GDCR with your code!

[Sresh Rangi]

Hello Liutauras,

I don't see what the Regenerable interface is adding. You could replace any call to regenerate like:

location -> mutableAt(location).regenerate(rules.withLivingNeighbors(countAt(location)).build())

with:

location -> rules.withLivingNeighbors(countAt(location)).build().apply(mutableAt(location))

and get the same effect.

When you use it inside the RegenerationRules class, then you take Regenerable as a parameter but you're comparing to concrete types which indicates that it's not a real abstraction.

For the rules, withLivingNeighbors is required to construct an instance of them. I would think that livingNeighbors is not part of the rules, but the world state that would be passed to the rules instance when it's used. So RegenerationRules would have a method like: Cell nextGeneration(int livingNeighbors).

The would allow the rules to be constructed outside the world and passed in so the world is decoupled from any specific rules instance. Also, I would allow the rules class to be subclassed, or configured in some way so you can have different rules.

[Liutauras Vilda]

First, pie slices to those who haven't got yet!!!

Some interesting points floating around, which in turn already allowed me to see some deficiencies with my current design. I need to absorb/experiment with each of these first.

But even now, I already can say a big thanks for your time and constructive criticism - I'll get back to each of you (salvin, Piet, Sresh), either late night today or other day once manage to spend some time on it.

[Gerard Charles]

Concur with Piet that it's perhaps overdesigned.

Cell seems to be an enum replacing what is functionally a boolean; the two states (ALIVE/EMPTY) aren't logically intuitive -- that is, I'd expect ALIVE/DEAD or FULL/EMPTY.  The symbol field seems to be mixing representative with logic (Not following Model-View-Controller).

If each location had a current state and a separate numNeighbors field, you wouldn't need to copy the map -- simply pass through twice. First calculate the number of neighbors, then make a second pass to update the alive/dead state.

Most importantly, Location has too much information about World in it. Make World an interface that tells each location who its neighbors are. This would only have to happen at program startup -- the neighbors don't change during execution.
Then:
1. Location simply iterates a collection to count alive neighbors.
2. You can change the shape of the world without changing Location. It could become rectangular with arbitrary dimensions, or even hexagonal instead of a square grid.  

[Liutauras Vilda]

salvin francis wrote:I looked at your code a bit more in-depth today and the following questions came to my mind :

Why can one not instantiate World ? The only way you are allowing is via a empty() method which generates according to a fixed SIZE of 10

Why is World square ? (as in number of rows=number of columns)

Well, one CAN instantiate world, and that is exactly through a method empty(). I think it is expressive and tells what one can expect from such a created world, meaning the world to be empty.
At this stage I did not concentrate much on such aspects as world's height, width, shape.. I might do care about them when I come to the moment of GUI need and in particular if I would want users to be able to set world size, but once again, at this stage my main goals were to experiment with design decisions, and so the different aspects of code could be tested throughout without exposing much/or at all of their internals, or in isolation if we can call it like that.

salvin francis wrote:
I see that you have implemented this as a fixed world with cells which are either Alive or Empty. The way I approached this was that I made an implementation which only kept Alive cells. There were no "Empty" cells.
Actually this is what fits in the real world philosophy too. In real life, we would observe a 'petri dish' with some cells. These cells would grow/die depending on various conditions, the 'petri dish' is their domain/world. With the implementation of "Empty" it looks like the world has some "slots" where a cell can reside or not.

Just food for thought

This your thought made me stop and think for a while. Sounds reasonable what you say. I see the way to avoid EMPTY and have only ALIVE, I'll try such approach as haven't tried yet. Thanks!


NOTE: (to those who follow)... The main reason of mine to start this project was/is to experiment with various designs implementing this game. I wouldn't say there is obviously good or obviously bad design(s). And I wanted to try some of these (designs) before I attend GDC event, so I would have some of my own prior experience in order to be more productive during the event.

[Liutauras Vilda]

Piet Souris wrote:I also had a look at your code. My first impression is that I get a sense of over-desinging here and there, making things more complex than necessay. Perhaps it is the modern way of dong, I don't know, but for instance this Builder.build() looks a bit cumbersome.
...
About the RegenerationRules, why do you fix the 2 and the 3, given that the class is final? I had a similar class, with two fields: List<Integer> stayAlive and ditto: becomeAlive, that were parameters of my constructor.

Yes, probably it was over-designed here. It was really a wacky builder pattern implementation (with an idea to augment), which in turn served very little.

The thought behind that was, that I took some hypothetical future requirement of some extra rules let's say:

RegenerationRules.Builder rules = new RegenerationRules.Builder();
rules.withLivingNeighbors(..).withBrickWalls(..).withDragons(..)..

And I thought I could let them change over the moment in time...
But since I did not attempt to implement non of these extra's, I most likely over-complicated without seeing obvious deficiencies and went down the wrong route. Yet more experiments to come. Thanks!

Piet Souris wrote:...determine the neighbors of a Location... but that method is not foolproof...

Good point, missed it. Thanks for the comments!

Piet Souris wrote:But please report your impressions at the GDCR with your code!

Indeed these were my goals to have some prior experiments done as my homework, so could have more productive time there after being fallen to few traps already!

[Liutauras Vilda]

Sresh Rangi and Gerard Charles, will get back to you later... seeing some good comments

[Ganesh Patekar]

Congratulations Liutauras,

Your question has made it to our Journal  

Have a Cow!

[Liutauras Vilda]

Sresh Rangi wrote:
I don't see what the Regenerable interface is adding. You could replace any call to regenerate like:

location -> mutableAt(location).regenerate(rules.withLivingNeighbors(countAt(location)).build())

with:

location -> rules.withLivingNeighbors(countAt(location)).build().apply(mutableAt(location))

Indeed it can be replaced. Interesting enough, but such redundancy/duplication even were well visible within the test classes: RegenerableTest and RegenegerationRulesTest. Once I removed such redundancy, one of the test class also got removed. Codebase became more compact and simpler.

However, I still keeping my Regenerable interface as I do need to have some common type to be able to manipulate with. Will experiment further.

Sresh Rangi wrote:For the rules, withLivingNeighbors is required to construct an instance of them. I would think that livingNeighbors is not part of the rules, but the world state that would be passed to the rules instance when it's used.

Yes, after some extra experiments I see some existing deficiences with my current RegenerationRules. I have a possible idea of improvement, however, it isn't something major I'm thinking of currently.

Sresh Rangi wrote:Also, I would allow the rules class to be subclassed, or configured in some way so you can have different rules.

Not a fan of letting to subclass by default, but do agree about the second part, that's exactly why initially created a rules builder class to be able to assemble various rules on demand (except that I got that a bit wacky), which will improve as per above.

Thankful for your thoughts.

[Liutauras Vilda]

Gerard Charles wrote:Cell seems to be an enum replacing what is functionally a boolean; the two states (ALIVE/EMPTY) aren't logically intuitive -- that is, I'd expect ALIVE/DEAD or FULL/EMPTY.

I attempted with boolean right from the beginning, but later realised it was very limited approach, hence refactored. However, currently after some extra experiments I got only an ALIVE instance of a cell due to comments mentioned ealier.

Gerard Charles wrote:The symbol field seems to be mixing representative with logic (Not following Model-View-Controller).

Well, this project was not meant to have GUI, at least at that moment, so I really had that part just to ease up testing by printing them (cells).

Gerard Charles wrote:If each location had a current state and a separate numNeighbors field, you wouldn't need to copy the map -- simply pass through twice. First calculate the number of neighbors, then make a second pass to update the alive/dead state.

Most importantly, Location has too much information about World in it. Make World an interface that tells each location who its neighbors are. This would only have to happen at program startup -- the neighbors don't change during execution.

I'm not entirely sure I understand you exactly, but maybe in my latest revision I do something similar as I experimented with few other approaches.

Well, about Location I'd say opposite, it has no information about the World almost at all (apart from the Wold.SIZE, but even that would become uneccessarry if I'd mimmic a real infinite size world; size I introduced for the purpose to wrap evolution round some sort frame). I like Location class (after I fix a possible outside-boundaries problem), because it is really represents one thing and I think it does well; It has only one reason to change.

Thanks for comments.

[Liutauras Vilda]

Thank you all for the comments once again and a lot. They helped me to try various approaches, do more and more refactorings, see some deficiencies of my previous/current and possibly future approaches. Now I came to the fields, where it is really hard to say which approach was/is better, which - worse. Most importantly, I tried many of them already, and saw how code changes, how test suite changes. It certainly taught me quite a few lessons. I guess this project going to be a long lasting of mine where I'm going to try new things continuously. So the version you see now, might be outdated already after you have a tea/coffee

Thanks all!

[Junilu Lacar]

Late to the party, as usual. I will have a look and share my thoughts. In the meantime, I hope you're doing this in preparation for Global Day of Coderetreat which is less than two weeks away as of today. If not, please consider signing up if there's one being hosted near you.

[Junilu Lacar]

First, I want to say that I'm very happy to see this (the post, the forum, the feedback) is a safe place. What I mean by that is that criticism can be given in the spirit of learning and improvement. I see egoless programming being practiced here and that is awesome.

Piet Souris wrote:I also had a look at your code. My first impression is that I get a sense of over-desinging here and there, making things more complex than necessay. Perhaps it is the modern way of dong, I don't know, but for instance this Builder.build() looks a bit cumbersome.

I got the same impression as well after a few minutes of going over the code.

[Junilu Lacar]

Looking at the RegenerationRulesTest and RegenerationRules classes, I get a sense that you started with the Builder pattern in hand and went out looking for a nail to pound with that hammer. The result was these two classes.

The first indication of a problem to me was the existence of only two methods in your Builder. Well, one method, if you exclude the required build() method. The Builder pattern is normally applied when you have a constructor that contains complex initialization logic and takes many parameters, i.e., you have the Long Parameter List code smell. The code I see doesn't look like it addresses a problem with complex initialization. In fact, it kind of looks like you've actually introduced that problem by using the Builder pattern. That would make this usage an anti-pattern.

I also sense a mis-assigned responsibility here. Why do you need a separate class to encapsulate the rules of the game? I don't understand the semantics of this: rules.withLivingNeighbors(2).build().apply(Cell.ALIVE) -- reading the test code doesn't tell me a story about this call. The call just seems to come out of nowhere in the test.

When I want to get familiar with some code, the first thing I look at are the tests. I expect the tests to start telling me a story about the design. I don't get what story these names are trying to tell:

rules_with_two_alive_neighbors_applied_to_alive_cell_returns_alive_cell()
rules_with_three_alive_neighbors_applied_to_alive_cell_returns_alive_cell()
rules_with_three_alive_neighbors_applied_to_unpopulated_cell_returns_alive_cell()
...

These names seem to have a heavy focus on implementation. Why would a Rules object have alive neighbors? That's the meaning I get from "rules with X alive neighbors." And why would a Rules object return a Cell? The semantics just aren't that intuitive to me.

Here's what might have made sense to me instead:


live_cell_with_three_neighbors_lives_on_in_next_generation()
live_cell_with_two_neightbors_lives_on_in_next_generation()
empty_cell_with_exactly_three_neighbors_comes_alive_in_next_generation()

[Junilu Lacar]

Your World and Location classes do look promising though, except I don't get the semantics you have with those right now either.

I wrote:
Here's what might have made sense to me instead:

live_cell_with_three_neighbors_lives_on_in_next_generation()
...

If you had a test named like this, I would have expected to see something like this in the body of the test:

@Test
public void live_cell_with_three_neighbors_lives_on_in_next_generation() {
    Location[] neighbors = new Location[] {
         Location.of(1, 1), 
         Location.of(1, 2), 
         Location.of(1, 3)
    };

    Location testCell = Location.of(2, 2);

    World world = World.empty();
    World.makeLive(testCell);
    World.makeLive(neighbors);

    world.tick();

    assertTrue(world.isAlive(testCell));
}

Do you see how the test code tells a story of how a World and Location objects are related?

[Junilu Lacar]

I like the idea of a Location object. This is in line with what Corey Haines discusses in his book Understanding the Four Rules of Simple Design about reifying the cell coordinates.

I get the semantics of the Location.neighbor(Direction) method but I think you went too far and twisted a brain ankle there with Direction.neighborOf(Location). That looks like a Double Dispatch pattern hammer looking for a nail to pound. (Yes, I'm experimenting with the "got a hammer and looking for nails to pound with it" metaphor for design pattern anti-patterns)

I'd like to see you work on refactoring the LocationTest and Location classes a little bit. For example, this test:

    @Test
    public void two_same_locations_are_equal() {
        assertEquals(Location.of(0, 0), Location.of(0, 0));
    }

I get what you're trying to say here but it raises at least one question about the design: Does the Location.of() factory method guarantee that discrete instances of Location will be returned? That seems to be the tacit/implied assumption made by this test.

What if somebody later on decided to cache common Location objects, like how the standard Java library caches certain Integer objects? Wouldn't this test break if Location.of(0, 0) returned a cached object instead? [Edit: I guess I'll answer my own question here: it wouldn't break the test because equals() should, in fact, return true if you are comparing an object to itself. My train of thought was just running into itself.] If you're going to design a Factory method that is guaranteed to return unique instances of objects, then your test should document that explicitly.

private Random rand = new Random();

@Test
public void of_returns_discrete_instances_with_same_coordinates() {
    int x = rand.nextInt(100);
    int y = rand.nextInt(100);

    assertNotSame(Location.of(x, y), Location.of(x, y));
}

@Test 
public void equals_is_true_when_same_coordinates() {
    int x = rand.nextInt(100);
    int y = rand.nextInt(100);

    assertEquals(Location.of(x, y), Location.of(x, y));
}

These tests are independent but also very much related to each other and I would put them in close together in LocationTest. Together, they tell a story that answers the questions I have about the design choice you made.

[Junilu Lacar]

On the other hand, if you choose to design your Location.of() to return cache values of certain coordinates, then your tests should explicitly document that decision and the details as well:

@Test
public void of_returns_cached_instances_from_0_0_to_10_10() {
    // ... left to you as an exercise
}

@Test
public void of_returns_discrete_instances_for_coordinates_beyond_10_10() {
    // ... left to you as an exercise
}

@Test
public void equals_is_true_when_same_coordinates() {
    // ... same as in previous reply
}

[Junilu Lacar]

Looked over previous comments and found those made by Piet and Gerald interesting and worth pursuing, which I think you did: you took out knowledge about World from Location and Location.Direction. That's a good start.

Here's another way to push your design and see if there's some more refactoring you can do. I have a sense that there is. Think about having a World that represents a toroidal coordinate system. That is, given a World that is 100 x 100 spaces, then Location.of(0, 0) would be neighbors with (99, 99), (99, 0), (99, 1), (0, 99), (1, 99) as well as the obvious (0, 1), (1, 1), and (1, 0). What would you need to change in your program to account for toroidal cell coordinates like this? How easy/difficult would it be to introduce that change?

[Piet Souris]

Junilu Lacar wrote:Looked over previous comments and found those made by Piet and Gerald interesting and worth pursuing, which I think you did: you took out knowledge about World from Location and Location.Direction. That's a good start.

Here's another way to push your design and see if there's some more refactoring you can do. I have a sense that there is. Think about having a World that represents a toroidal coordinate system. That is, given a World that is 100 x 100 spaces, then Location.of(0, 0) would be neighbors with (99, 99), (99, 0), (99, 1), (0, 99), (1, 99) as well as the obvious (0, 1), (1, 1), and (1, 0). What would you need to change in your program to account for toroidal cell coordinates like this? How easy/difficult would it be to introduce that change?

Interesting. Initially Liutauras had indeed implemented toroidal coordinates, see my remark about the use of the modulo operator. But it looks as if that has been removed.

[Junilu Lacar]

I forgot to mention this earlier but I really like the choice of name for the Location.of(int, int) factory method. It is symmetrical with other such methods that have been introduced to the Standard Java Library. Making your API symmetrical with well-known APIs is a good way to make your code more intuitive to read and understand. People who are familiar with the well-known API can use their experience to draw parallels with your API and expect similar behavior. This is a double-edge sword though. You have to make sure you don't break the Principle of Least Astonishment (POLA) by making your method behave in any unexpectedly different ways from the well-known API's behavior.

[Junilu Lacar]

I also like the semantics of code like this:

World emptyWorld = World.empty();

It immediately makes me think of being able to do functional programming with this API. Something like this perhaps:

public class World {

   private List<Location> occupied = ...;

   public World tick() {  
        return World.empty().addAll(nextGeneration());
   }

   public World addAll(List<Location> occupiedCells) {
        ...

        return ...
   }

   private List<Location> nextGeneration() {
        List<Location> population = new ArrayList<>();

        // calculate all cells that will be occupied in
        // next generation based on current population
        ... 

        return population;
   }

   ...
}

I think I'll give this as a challenge for GDCR this year: Write a functional implementation of GoL where you're not changing the state of any object. That is, all objects created in your implementation should be immutable.

[Junilu Lacar]

I think I pulled a brain muscle trying to reach an understanding of what the code in Regenerable.java is trying to say.

What are you trying to say with that code? Can you please explain your thought process there?

[Junilu Lacar]

Liutauras Vilda wrote:The thought behind that was, that I took some hypothetical future requirement of some extra rules let's say:...

(Emphasis is mine) The key words that should raise a red flag and start waving it crazily around. Avoid doing that unless you are truly a psychic who can foresee the future. Otherwise, you are just adding unnecessary complexity to your design to accomodate a future that may never materialize. Why waste time and effort doing that? Keep things simple and clean and well-factored enough so that when you do get new requirements, it will be easy to change your code and adapt it to exhibit the new behaviors you need.

[Liutauras Vilda]

Junilu Lacar wrote:Late to the party, as usual. I will have a look and share my thoughts. In the meantime, I hope you're doing this in preparation for Global Day of Coderetreat which is less than two weeks away as of today. If not, please consider signing up if there's one being hosted near you.

It is not too late, you are right on time, as you mentioned, Global Day of Coderetreat is approaching (next Saturday), and I'm indeed enrolled (thanks for introducing us to this event in general, that was last year), so this project and the discussion you are all participating in is an ideal way to get ready to take most from it, and hopefully give as much as I can to my peer(s). My version of GoL is meant to evolve for as long as I find something to improve, apply different techniques, learn new things, experiment with "other" ideas, so it is never too late actually.

I've assigned you a pie slice (as to everyone else), that's my little gratitude and BIG thanks for spending your time reviewing my code. Once again, I'm equally thankful to everybody.

Now, let's get back to work code One note, I didn't modify code since the moment Junilu's comments were written, that's on purpose, so the parts could be discussed before I move on with improvements (other comments I think I've addressed, at least to some extent).

(to be continued in next reply).  Since Junilu never go "small", meaning, singular post is never a case, I'll need to be careful not to miss a post among his N-1 posts

[Liutauras Vilda]

One administrative note, I've added "markings" to not yet addressed posts (as #1; #2...; #10), so would be easier to refer to them as I'm going to come back to them probably not in order.

[Liutauras Vilda]

#5 (by Junilu), #6 (by Piet).

Junilu Lacar wrote:Think about having a World that represents a toroidal coordinate system... What would you need to change in your program to account for toroidal cell coordinates like this? How easy/difficult would it be to introduce that change?

Piet Souris wrote:Interesting. Initially Liutauras had indeed implemented toroidal coordinates, see my remark about the use of the modulo operator. But it looks as if that has been removed.

You both rightly identified my initial problem of having Location class polluted with knowledge about the World, and in particular its size. But it caused me several problems, one of which what Piet mentioned. As you understand in general, I did for a reason to be able to represent an "infinity" world. However, it is not that it isn't handled now, it is, just in a different place - in a World's class. That gave me an opportunity to have Location class decoupled from World and leave World to worry about its size/shape.

From World class:

public void aliveAt(Location location) {
	worldMap.put(Objects.requireNonNull(
			worldWrapped(Location.of(
					Math.abs(location.rowIndex()),
					Math.abs(location.columnIndex())))),
			Cell.ALIVE);
}

private Location worldWrapped(Location location) {
	return Location.of((location.rowIndex() + size) % size,
			(location.columnIndex() + size) % size);
}

The code is not documented yet, but with current implementation the World is wrapped. If let's say the World is of size 10, so by setting Location.of(1, 1) or Location.of(11, 1) or Location.of(-11, 1), it represents the same location within such world, so I think that addresses the issue Piet pointed out where indices could go out of bounds.

Living neighbours also are retrieved using worldWrapped concept what Junilu mentioned:
.map(neighbor -> worldMap.get(worldWrapped(neighbor))) (World.java:67)

At least at the moment, I think I have it better handled than before.

[Liutauras Vilda]

#9 (by Junilu)

Junilu Lacar wrote:I think I pulled a brain muscle trying to reach an understanding of what the code in Regenerable.java is trying to say.

What are you trying to say with that code? Can you please explain your thought process there?

Well, you aren't the first who point finger to this one. But somehow I still disagree with everybody on this. I just want you all please understand, that I don't want to do any changes blindly just because somebody (presumably somebody more experienced) told me, I want to convince myself first with such change if I go along with.

My rationale behind that was to have a clear instrument to extend the system with more types. For example: Cell.MONSTER (that doesn't sound a big deal), how about Brick.RED, Brick.YELLOW, or new Tree(...), new Lion()... Well, in a case of Brick, probably a type's name Regenerable would become questionable...

Now, having such interface (if I have documented it yet), would be clear (at least looks like that now), that one needs to implement this common type (can I call it a "marker" interface here?) + update RegenerationRules (will get back to this in #1, that's a tough).

If I did not have such interface, I'd need to modify World class where Cell would have been refered, then RegenerationRules.

Plus I have tests written which referencing this type already, so by adding new types as for obstruction purpose for example, I wouldn't need to trash out my current tests, I would need just to add more tests to cover newly added types.

Now I tried to imagine let's say I get rid of this interface and simply have Cell everywhere, which is the only type I have now. So I write tests to achieve reasonable coverage, and then I get new types. So I need to either delete old tests or at least update/modify them.

My ideas above all this were, that I have 2 interfaces: Regenerable and RegenerationRules to implement when adding more types. But lately RegenerationRules made a concrete class rather than interface. <-- Now that I'm reading what I wrote just before, I get what Junilu is saying with trying to forsee the future. How do I know whether the possible extension is about the types? Or about renegeratiaon rules?

Ok, I need to revise that.

[Liutauras Vilda]

#10 (by Junilu)

Junilu Lacar wrote:

Liutauras Vilda wrote:The thought behind that was, that I took some hypothetical future requirement of some extra rules let's say:...

(Emphasis is mine) The key words that should raise a red flag and start waving it crazily around. Avoid doing that unless you are truly a psychic who can foresee the future. Otherwise, you are just adding unnecessary complexity to your design to accomodate a future that may never materialize. Why waste time and effort doing that? Keep things simple and clean and well-factored enough so that when you do get new requirements, it will be easy to change your code and adapt it to exhibit the new behaviors you need.

Yeah... I kind of tried to put myself in a mindset, that I have new requirements already, just not implementing them yet and working only with current (old) requirements, but with an idea that could extend with new.

But as you (and Piet) said - I made a mistake by overcomplicating system taking into account a non existing future.

Will revise the part about RegenerationRules.

[Liutauras Vilda]

#7, #8 (by Junilu)

Thanks for finding also some good words by mentioning places you liked.

Junilu Lacar wrote:I forgot to mention this earlier but I really like the choice of name for the Location.of(int, int) factory method. It is symmetrical with other such methods that have been introduced to the Standard Java Library...
This is a double-edge sword though. You have to make sure you don't break the Principle of Least Astonishment (POLA) by making your method behave in any unexpectedly different ways from the well-known API's behavior.

That's something important to remember, didn't think about that initially. If choose something what's well established in other API's, need to take responsibility representing same ideas. Will be remembered!

Junilu Lacar wrote:I think I'll give this as a challenge for GDCR this year: Write a functional implementation of GoL where you're not changing the state of any object. That is, all objects created in your implementation should be immutable.

That was my idea too, to have as less places mutable as possible.

[Liutauras Vilda]

For #1; #2; #3; #4 I need some more time to get back, especially with #1 - that seem to be my biggest problem at the moment.