I have been teaching myself programming, mainly in Java, for about 6 months. I'm looking for some feedback on this implementation of the Game of Life, which runs entirely in the terminal. Specifically, where does it not follow best practices, is it over-engineered, etc?
The entry point:
public class GameOfLife {
public static void main(String[] args) {
Seed seed = new OscillatorSeed(); //could be any object implementing seed interface
LifeBoard gameOfLife = new LifeBoard(seed);
}
}
Seed interface defines specific behavior of concrete seeds:
public interface Seed {
/*
*pattern for seed files
*must have a "board" made of a 2d boolean array
*board is not required to be rectangular
*/
public int getHeight();
public int getWidth();
public boolean[][] getSeed();
}
Here are some concrete seeds:
public class OscillatorSeed implements Seed {
public boolean[][] seed = {
{false, false, false, false, false},
{false, false, false, false, false},
{false, true, true, true, false},
{false, false, false, false, false},
{false, false, false, false, false}
};
public int height = 5;
public int width = 5;
public int getHeight() {
return height;
}
public int getWidth() {
return width;
}
public boolean[][] getSeed() {
return seed;
}
}
public class GliderSeed implements Seed {
public boolean[][] seed = {
{false, false, false, false, false},
{false, false, false, true, false},
{false, true, false, true, false},
{false, false, true, true, false},
{false, false, false, false, false}
};
public int height = 25;
public int width = 40;
public int getHeight() {
return height;
}
public int getWidth() {
return width;
}
public boolean[][] getSeed() {
return seed;
}
}
BoardPrinter has one static method, print, to paint the game board in the terminal after each generation. It is tailored to my specific console, and clips large boards to fit or inserts blank space for small boards, so the output at each refresh is exactly my console's number of lines-1 each refresh, makes it look smoother. This is my attempt to separate the UI/display code from the model of the game. IS this a reasonable way to do it?
public class BoardPrinter {
public static int TERM_ROWS = 24;
//xterm leaves last line blank, remember to -1 num_rows
static void print(boolean[][] state) {
// debugging code only: System.out.println("" + state.length + state[0].length);
if (state.length <= TERM_ROWS) {
for (int row = 0; row < state.length; row++) {
for (int position = 0; position < state[row].length; position++) {
if (state[row][position] == true) {
System.out.print("*");
} else {
System.out.print("~");
}
}
System.out.print("\n");
}
} else {
for (int row = 0; row < TERM_ROWS-1; row++) {
for (int position = 0; position < state[row].length; position++) {
if (state[row][position] == true) {
System.out.print("*");
} else {
System.out.print("~");
}
}
System.out.print("\n");
}
}
try {
//100 ms delay = 20 fps
Thread.sleep(100);
} catch (Exception e) {
e.printStackTrace();
}
}
}
The actual implementation of the game, rules, etc. I know the block of if statements should be made more efficient. I would like to do it with a loop of some sort but am still figuring out how to do that. I'm considering making an array with each of the 8 enums in it to use in a for-loop. Is the use of enum even a reasonable way to do the direction-search? What else am I not considering?
public class LifeBoard {
public boolean[][] oldState;
public boolean[][] newState;
public boolean[][] blankState;
private boolean gameRunning = true;
public LifeBoard(Seed seed) {
oldState = new boolean[seed.getHeight()][seed.getWidth()];
newState = new boolean[oldState.length][oldState[0].length];
for (int i = 0; i < seed.getSeed().length; i++) {
for (int n = 0; n < seed.getWidth(); n++) {
System.arraycopy(seed.getSeed()[i], 0, oldState[i], 0, seed.getSeed().length);
}
}
blankState = new boolean[oldState.length][oldState[0].length];
run();
}
public void run() {
//game runs forever, evaluateCells() should switch the game off when it detects no change for consecutive generations, but I haven't figured out how to implement that yet
gameRunning = true;
while (gameRunning) {
BoardPrinter.print(oldState);
gameRunning = evaluateCells();
for (int n = 0; n < oldState.length; n++) {
System.arraycopy(newState[n], 0, oldState[n], 0, oldState.length);
}
for (int t = 0; t < newState.length; t++) {
System.arraycopy(blankState[t], 0, newState[t], 0, newState.length);
}
}
BoardPrinter.print(oldState);
System.out.println("game over");
}
public boolean evaluateCells() {
for (int row = 0; row < oldState.length; row++) {
for (int position = 0; position < oldState[row].length; position++) {
newState[row][position] = evaluateCell(row,position);
}
}
//return true while life is still evolving, when board is static, return false
//feature not yet implemented
return true;
}
public boolean evaluateCell(int row, int position) {
int score = 0;
//evaluate each neighboring cell
if (getCell(row, position, Direction.ToRight)) {
score += 1;
}
if (getCell(row, position, Direction.ToLeft)) {
score += 1;
}
if (getCell(row, position, Direction.ToTop)) {
score += 1;
}
if (getCell(row, position, Direction.ToBottom)) {
score += 1;
}
if (getCell(row, position, Direction.ToBottomRight)) {
score += 1;
}
if (getCell(row, position, Direction.ToBottomLeft)) {
score += 1;
}
if (getCell(row, position, Direction.ToTopRight)) {
score += 1;
}
if (getCell(row, position, Direction.ToTopLeft)) {
score += 1;
}
//evaluate total of score against rules of the game
//implimentation of game rules
if ((oldState[row][position] == true) && (score < 2)) {
return false; //die of loneliness
} else if ((oldState[row][position] == true) && (score == 2 || score == 3)) {
return true; //continue to live
} else if ((oldState[row][position] == true) && (score > 3)) {
return false; //die of overcrowding
} else if ((oldState[row][position] == false) && (score == 3)) {
return true; //life is born
} else {
return false;
}
}
public boolean getCell(int row, int position, Direction direction) {
int newY = row+direction.getYDir();
int newX = position+direction.getXDir();
if (newX < 0 || newX >= oldState[row].length) {
return false;
}
if (newY < 0 || newY >= oldState.length) {
return false;
}
return oldState[newY][newX];
}
public enum Direction {
ToRight(1,0),
ToLeft(-1,0),
ToBottom(0,1),
ToTop(0,-1),
ToTopRight(1,-1),
ToTopLeft(-1,-1),
ToBottomRight(1,1),
ToBottomLeft(-1,1);
private int xDir;
private int yDir;
private Direction(int xDir, int yDir) {
this.xDir = xDir;
this.yDir = yDir;
}
public int getXDir() {
return xDir;
}
public int getYDir() {
return yDir;
}
}
}
All this code runs on my machine, no obvious problems, and I get the expected results. In the future I would like to add a SeedFactory that takes a CSV text file and builds a Seed from that, rather than compiling a separate class for each seed. I would specify java GameOfLife seedfile.seed, instantiate the factory in the GameOfLife class, and pass the arg to the factory, let it return a Seed, and pass that to the LifeBoard. A bloated way of doing it?
2 Answers 2
First of all, I think your code is very nice for 6 months of training. The following tips are mainly regarding style and object orientation.
- If you want to truly separate the implementation from the user interface, you should not call
BoardPrinter.print()from within yourLifeBoardclass. A good way to achieve separation is, for example, to create an interface (for instanceIBoardOutput), which has a methodprint(boolean[][] state). Then, you can add a method to yourLifeBoardclass, which allows you to set anBoardOutput, soBoardOutput.print(...)can be called. Finally, letBoardPrinterimplement the interfaceIBoardOutput. So, the output method can be changed without touching theLifeBoardclass. This is called strategy pattern or dependency injection, if you want to do further research. - Be careful with data encapsulation. Your
BoardPrinterclass receivesoldGamefield, which enables theBoardPrinterclass to modify your gamefield! If you implement the approach above, an implementation ofBoardOutputcould modify the gamefield by accident. Passing a copy of the gamefield toBoardOutputis possible solution, passing an instance ofLifeBoardistself and accessing thegetCellmethod is another one (add an overload without the direction parameter for this). - Do the
evaluateCellandevaluateCellsmethods really have to be public? - Your implementation of
Directionis nice, if you have to access methods which depend on theDirectionclass from outside. However, I only see this class being used internally. In such a case, two nested for loops from -1 to 1 for searching directions would be fine, too, especially if performance is important. If you need a code sample on how to do this, just leave a comment. - Instead of
if(state[row][position] == true)useif(state[row][position]). - If you want a small pause between the steps of the simulation, DON'T pause the thread in the
BoardPrinterclass. Do it in yourLifeBoardclass. Why? Because theBoardPrinteris for printing the board, not for controlling the simulation. - Implementing a
SeedFactoryis a valid way of doing so, and will probably be a nice exercise. - Your
OscillatorSeedclass exposes variables as public (public int width;). You have getters for that and should make them private (or even const/final, since these values are not supposed to change any more).
Fun tip: You can even use the mentioned Strategy pattern to make the set of rules used for the game exchangeable.
Have fun!
I thought it would be fun if I modified your program the way I would do it and posted that code. I haven't really interacted with other programmers so I don't really know if what I am doing is normal, but I will explain my reasoning.
GameOfLife
Let's start with the main class:
public class GameOfLife {
public static void main(String[] args) {
Seed seed = new OscillatorSeed(); //could be any object implementing seed interface
LifeBoard lifeBoard = new LifeBoard(seed);
while (true) {
lifeBoard.iterate();
BoardPrinter.print(lifeBoard);
waitForNextFrame();
}
}
static private void waitForNextFrame() {
try {
//100 ms delay = 20 fps
Thread.sleep(100);
} catch (Exception e) {
e.printStackTrace();
}
}
}
There are a few things I did here.
First, I renamed our LifeBoard object lifeBoard. Previously it was named gameOfLife. The old name doesn't make sense because the object is of type LifeBoard and not of type GameOfLife. Choosing bad names for objects can make the code confusing so watch out for that.
Second, I have moved the logic of controlling the simulation out into the main GameOfLife class. This is because the object that is responsible for iterating the system should not also be responsible for figuring out when and for how long it should iterate, and it should not be responsible for determining when it should draw itself.
A third thing I did was to extract waiting for the next frame into its own function. This is because it is a conceptually simple thing that can be separated out easily.
Seed
Next let's look at the seed interface:
public interface Seed {
/*
*pattern for seed files
*must have a "board" made of a 2d boolean array
*board is not required to be rectangular
*/
public int getNumRows();
public int getNumColumns();
public boolean[][] getSeed();
}
All I have done here is to change the names of the getters to getNumRows() and getNumColumns(). Before it was getHeight() and getWidth(). I choose the new names because I want everything to be referred to by rows and columns uniformly throughout the whole program. Before you had width, x and position, where now I will just say column, and you had height, y, and row, where now I will just say row.
OscillatorSeed
public class OscillatorSeed implements Seed {
public boolean[][] seed = {
{false, false, false, false, false},
{false, false, false, false, false},
{false, true, true, true, false},
{false, false, false, false, false},
{false, false, false, false, false}
};
private final int numRows = 5;
private final int numColumns = 5;
@Override
public int getNumRows() {
return numRows;
}
@Override
public int getNumColumns() {
return numColumns;
}
@Override
public boolean[][] getSeed() {
return seed;
}
}
Besides changing the names of the variables, I have also made them private and final, since they aren't supposed to be changed, and we can access them with the getters. I did the same thing for GliderSeed, so I won't post the code for GliderSeed here. seed should probably also be private and final.
BoardPrinter
Let's move on to the BoardPrinter class
public class BoardPrinter {
public static int TERM_ROWS = 24;
//xterm leaves last line blank, remember to -1 num_rows
static public void print(LifeBoard lifeBoard) {
// debugging code only: System.out.println("" + state.length + state[0].length);
final int numRowsPrinted = Math.min(lifeBoard.getNumRows(), TERM_ROWS);
final int numColumns = lifeBoard.getNumColumns();
Cell currentCell = new Cell(0, 0);
for (currentCell.row = 0; currentCell.row < numRowsPrinted; currentCell.row++) {
for (currentCell.column = 0; currentCell.column < numColumns; currentCell.column++) {
final boolean isAlive = lifeBoard.isAlive(currentCell);
printCell(isAlive);
}
endLine();
}
}
static private void printCell(boolean isAlive) {
if (isAlive) {
System.out.print("*");
} else {
System.out.print("~");
}
}
static private void endLine() {
System.out.println("\n");
}
}
The first thing you can see is that I am now passing the whole lifeBoard object to the method. This way we don't have to expose the boolean array, which Emiswelt said would be a bad idea. I will show how I access the values of the array when I get to explaining that part.
The next thing is I got rid of your outer if by figuring out how many rows I wanted to print first. Then I just use that number. A good way to tell that your outer if was probably not the best way of doing things is that the body of the if black was the same as the body of the else block. That kind of code duplication is a sure sign that something can be improved.
You can see I created a Cell class to encapsulate a pair of ints. I will talk about this later. I use an object of this class to access the boolean array in lifeBoard. Since I am using a getter which returns a boolean, there is no chance of me altering the state of lifeBoard.
The next thing I did was to extract printCell to be its own method. This separates the logic of looping through the cells from the logic of deciding which character to draw. It also makes the print function look more manageable, and the name of the method tells you what is going on, which would be harder to tell just from looking at the if-else the way you had it. Notice I have gotten rid of two levels of indentation, which makes the method easier to read.
Then I did something kind of crazy, which was to extract printing the newline character to its own function. The idea is if printing characters should have its own when I am printing the cell, then it should have its own method when I print a newline. Doing it this way has the benefit of saying why I am printing the newline character. It's because I am ending the line. This is a minor point.
LifeBoard
Last but not least we get to the LifeBoard class.
public class LifeBoard {
static public class Cell {
public int row;
public int column;
public Cell(int row, int column) {
this.row = row;
this.column = column;
}
public Cell getNeighbor(Direction direction) {
return new Cell(row + direction.getRowChange(), column + direction.getColumnChange());
}
}
static public enum Direction {
RIGHT(1, 0),
LEFT(-1, 0),
DOWN(0, 1),
UP(0, -1),
UP_RIGHT(1, -1),
UP_LEFT(-1, -1),
DOWN_RIGHT(1, 1),
DOWN_LEFT(-1, 1);
private final int columnChange;
private final int rowChange;
private Direction(int columnChange, int rowChange) {
this.columnChange = columnChange;
this.rowChange = rowChange;
}
public int getColumnChange() {
return columnChange;
}
public int getRowChange() {
return rowChange;
}
}
private final boolean[][] oldState;
private final boolean[][] newState;
private final int numRows, numColumns;
public LifeBoard(Seed seed) {
numRows = seed.getNumRows();
numColumns = seed.getNumColumns();
oldState = new boolean[numRows][numColumns];
newState = new boolean[numRows][numColumns];
for (int row = 0; row < seed.getNumRows(); row++) {
System.arraycopy(seed.getSeed()[row], 0, oldState[row], 0, seed.getNumColumns());
}
}
public void iterate() {
updateCells();
copyNewToOld();
}
private void copyNewToOld() {
for (int row = 0; row < numRows; row++) {
System.arraycopy(newState[row], 0, oldState[row], 0, numColumns);
}
}
public boolean updateCells() {
Cell currentCell = new Cell(0, 0);
for (currentCell.row = 0; currentCell.row < numRows; currentCell.row++) {
for (currentCell.column = 0; currentCell.column < numColumns; currentCell.column++) {
updateCell(currentCell);
}
}
//return true while life is still evolving, when board is static, return false
//feature not yet implemented
return true;
}
public void updateCell(Cell cell) {
final int numLivingNeighbors = numLivingNeighbors(cell);
updateCellWithNeighbors(cell, numLivingNeighbors);
}
private int numLivingNeighbors(Cell cell) {
int numLivingNeighbors = 0;
for (Direction direction : Direction.values()) {
if (isNeighborAlive(cell, direction)) {
numLivingNeighbors++;
}
}
return numLivingNeighbors;
}
private boolean isNeighborAlive(Cell cell, Direction direction) {
final Cell neighborCell = cell.getNeighbor(direction);
if (isInBounds(neighborCell)) {
return isAlive(neighborCell);
} else {
return false;
}
}
public boolean isInBounds(Cell cell) {
final boolean isRowInBounds = (cell.row >= 0) && (cell.row < numRows);
final boolean isColumnInBounds = (cell.column >= 0) && (cell.column < numColumns);
return isRowInBounds && isColumnInBounds;
}
private void updateCellWithNeighbors(Cell cell, int numLivingNeighbors) {
//evaluate total of score against rules of the game
//implimentation of game rules
if (isAlive(cell)) {
if (numLivingNeighbors < 2) {
clearCell(cell); //die of loneliness
} else if (numLivingNeighbors > 3) {
clearCell(cell); //die of overcrowding
} else {
setCell(cell); //continue to live
}
} else if (numLivingNeighbors == 3) {
setCell(cell);
} else {
clearCell(cell);
}
}
private void setCell(Cell cell) {
newState[cell.row][cell.column] = true;
}
private void clearCell(Cell cell) {
newState[cell.row][cell.column] = false;
}
public boolean isAlive(Cell cell) {
return isAlive(cell.row, cell.column);
}
public boolean isAlive(int row, int column) {
return oldState[row][column];
}
public int getNumRows() {
return numRows;
}
public int getNumColumns() {
return numColumns;
}
}
First I have added a static nested class called Cell. This class is convenient because rows and columns go together. Putting these two things together into one object will simplify method signatures, reducing clutter. I also added a method which takes a direction and returns the neighbor in that direction. I thought the Cell class was a good place to put that because the method doesn't rely on any of the logic in the rest of the class.
In the Direction Enum, I changed Top to Up and bottom to down, since up and down are more nearly directions than top and bottom. It seems like a small thing but having good names can really make coding easier.
I changed the fields of the class. I removed blankState because it was never used. I got rid of gameRunning because a LifeBoard object shouldn't be responsible for keeping track of when it is running. I added final ints for the number of rows and columns so we would never have to look those up again.
In the constructor, I got rid of the inner for loop, which was unnecessary, since you are copying the whole row at once instead of entry by entry.
Next is my iterate method, which roughly replaces your run method. Except my iterate method does much less. It is not responsible for drawing or waiting for the next frame or looping indefinitely. It just populates the newState boolean array with its new values and then copies that to the oldState array. I have cleanly split the function into these two parts. The implementation of the copy method is simple.
The updateCells method is easy to understand: I loop through all cells and update them one by one. Notice prefer a void updateCell method instead of having it return a boolean which I then use in an assignment with newState, which is the way you did it in evaluateCells. I am not sure if one is better, but at least the way I did it is another option.
updateCell, which is your equivalent of evaluateCell was cleanly broken in two peices. In the first I calculate the number of living neighbors of the given cell, and in the second I update the cell using that information.
The numLivingNeighbors method shows you how to loop over the values of an Enum. You were asking about this. I think this is a fine way to do it. Notice how numLivingNeighbors calls isNeighborAlive and isNeighborAlive calls isInBounds. Doing it this way keeps the methods short and easy to understand which is a good thing.
The last non-trivial method is updateCellWithNeighbors which still looks complicated, but the ifs are a little simplier because I have used nesting. There is one branch for if the cell is alive and one for if it isn't.
The remaining methods are just simple methods to make the syntax nicer.
Overall, I thought the design was good. As you can see I didn't add any new classes or change the responsibilities of the classes except to make the main class responsible for controlling the LifeBoard. I thought the code was really good for someone six months in. I think the take home messages here is that it is good to choose good names for variables, it is good to break big functions down into many little functions, and java has a special for each for loop syntax you can use (this is the one you use to iterate over values of enums). If you have any questions ask.