Problem
This is just a very simple version of the Game of Life made in Java. Please let me know how I could have improved this. I am also hoping for some feedback on how the formatting and conventions are because I would like my code to be readable and neat.
public class GameOfLife
{
private boolean[][] board;
private int maxRow, maxCol;
public GameOfLife(boolean[][] board)
{
if(board != null)
this.board = board;
else
this.board = new boolean[1][1];
this.maxRow = this.board.length;
this.maxCol = this.board[0].length;
}
public void calculateNextGeneration()
{
boolean[][] nextBoard = new boolean[maxRow][maxCol];
for(int row = 0; row < maxRow; row++)
{
for(int col = 0; col < maxCol; col++)
{
nextBoard[row][col] = nextState(row, col);
}
}
board = nextBoard;
}
public void print()
{
for(int row = 0; row < maxRow; row++)
{
for(int col = 0; col < maxCol; col++)
{
if(isAlive(row, col))
System.out.print("*");
else
System.out.print(".");
}
System.out.println();
}
}
public void setAlive(int row, int col, boolean isAlive)
{
if(inBounds(row, col))
board[row][col] = isAlive;
}
public boolean isAlive(int row, int col)
{
if(inBounds(row, col))
return board[row][col];
else
return false;
}
private boolean nextState(int rowPos, int colPos)
{
int alive = 0;
for(int row = rowPos - 1; row <= rowPos + 1; row++)
{
for(int col = colPos - 1; col <= colPos + 1; col++)
{
if(!inBounds(row, col) || (row == rowPos && col == colPos))
continue;
if(isAlive(row, col))
alive++;
}
}
if(alive == 3)
return true;
else if(isAlive(rowPos, colPos) && alive == 2)
return true;
else
return false;
}
private boolean inBounds(int row, int col)
{
if(row < 0 || row >= maxRow || col < 0 || col >= maxCol)
return false;
else
return true;
}
}
Solution
Thanks for sharing your code.
Naming
Finding good names is the hardest part in programming. So always take your time to think carefully of your identifier names.
Please read (and follow) the Java Naming Conventions
e.g.:
Your boolean variables and methods returning a boolean should start with is, has, can or alike.
Formatting
There is a discussion out there where to place the opening braces {.
You choose a separate line which is more common in other languages like C.
There might be valid reasons to do so but in Java the more common (yet not mandatory) position is on the same line with the proceeding statement. The three reasons why I follow this rule are:
- it saves one line per brace pair (making code looking more compact)
- I cannot accidentally place code between the statement and the opening brace which in most cases would detach the braced block from the preceding statement and so change the codes logic.
- most other Java coders do it.
There is s similar reasoning why I put the closing braces on a line of their own: It is quite likely that I actually want add more code inside a block and thus before a closing brace.
After all stick to either one rule throughout the program.
general approach
Your code is a procedural approach to the problem.
Procedural approaches are not bad of their own.
But Java is an Object Oriented programming language and if you want to become a good Java programmer you should start looking for more OO like solutions.
But OOP doesn’t mean to “split up” code into random classes.
The ultimate goal of OOP is to reduce code duplication, improve readability and support reuse as well as extending the code.
Doing OOP means that you follow certain principles which are (among others):
- information hiding / encapsulation
- single responsibility
- separation of concerns
- KISS (Keep it simple (and) stupid.)
- DRY (Don’t repeat yourself.)
- “Tell! Don’t ask.”
- Law of demeter (“Don’t talk to strangers!”)
i’m thinking similar – you can OO-out/ultimo, but for a mere boolean[][] you can be a bit tolerant ^_^ – Martin Frank
My point is: this should not be solved with a “mere boolean[][]“, especially if one wants to learn how to program Java.
I’m not saying that an OO approach is always best or even desirable…
In the case of my code, what should I have split into its own class? – bananashavings
-
In my OO world there would be a
Cellobject that maintains its own state and and holds aCollectionof its neighbors. It would inform its neighbors when its state changed. This way I only need to know the actual topology of the game field when assigning neighbors. I don’t even need to create an array if I find a smart way to assign the neighbors logically…For the rest of the game I’d hold the cells in a simple
Collection.
Consequences are:- separation of game control and business logic
- no range checks during iterations
-
Furthermore on a Game of Life board only a few cells (wild guessing less than 30% in average) are alive themselves or have alive neighbors. The rest will not change its state in the current iteration so I don’t need to process them.
In procedural approach based on a mere
boolean[][]there is no chance but processing all elements in each iteration.In a
Collectionbased approach I could optimize so that the collection only contains cells where its neighbors changed during last iteration by applying the listener concept.
just some minor things…
private boolean inBounds(int row, int col)
{
if(row < 0 || row >= maxRow || col < 0 || col >= maxCol)
return false;
else
return true;
}
could be more clear if you directly return the value
private boolean inBounds(int row, int col)
{
return (row >= 0 && row < maxRow && col >= 0 && col < maxCol)
}
the same matters for isAlive()
public boolean isAlive(int row, int col)
{
return inBounds(row, col) && board[row][col];
}
another thing is the name of the variable maxRows and maxColumns is not exactly what they are, i would use the simple forms for them rows and columns since they are rows and columns…
that would not even make conflicts for your iterations for(int row = 0; row < rows; row++)...
remove your whitespace, that makes reading easier…
First of all, your code looks quite good, much better than many other examples I’ve seen.
In addition to the other answers:
public GameOfLife(boolean[][] board)
{
if(board != null)
this.board = board;
else
this.board = new boolean[1][1];
// ...
}
If your caller gives you a null board array, you create a 1*1 board which surely isn’t useful to the caller.
I’d instead tell the caller that he has to provide a valid array, by throwing an e.g. InvalidArgumentException. Providing null is a bug in the calling code, and you should signal that bug as early as possible instead of continuing with nonsense data. If no-one in the caller stack knows how to continue from a null board, the exception will abort the program, and that’s good.
You use ifs without braces here and in many other places. That’s a maintenance trap. Maybe someone adds a line to one of the dependent blocks:
public GameOfLife(boolean[][] board)
{
if(board != null)
this.board = board;
else
area = 1;
this.board = new boolean[1][1];
// ...
}
At a first glance, that looks fine, and it’s easy to fall into the trap that now this.board = new boolean[1][1]; is executed unconditionally.
So I always use braces.
So, my version of your constructor would be:
public GameOfLife(boolean[][] board) {
if(board == null) {
throw new IllegalArgumentException("board must not be null.");
}
this.board = board;
this.maxRow = this.board.length;
this.maxCol = this.board[0].length;
}
Talking about conditional true/false returns vs. returning boolean expressions: I agree that e.g. inBounds() could be rewritten using one return statement with a boolean expression, but I typically do it your way. Why? It makes debugging easier. It’s easier to place a breakpoint on either the return true; or the return false; line than to make the single return breakpoint react conditionally on a true or false boolean value.