3

I have spent quite a while trying to write a program to implement Conway's game of life - Link with more info. . I am following some online guides and was given the majority of the functions. I wrote the "next" and "neighbours" methods shown below. Could anyone tell me if these are good implementations, and how they could be made better please ?

The point of the exercise was to not modify or change any of the other methods and just write the next method ! :)

import java.io.*;
import java.util.Random;

public class Life {

private boolean[][] cells;

public static void main( String[] args ) {
  Life generation = new Life( );
  for (int i = 0; i != 10; i++) {
    System.out.println( generation );
    generation.next( );
  }
}
// Constructors

public void next (){

  int SIZE;
  SIZE=cells.length;
  boolean[][] tempCells = new boolean [SIZE] [SIZE]; 

  for( int i=0; i<SIZE; i++ ) {
 for( int j=0; j<SIZE; j++ ) {
  tempCells[i][j] = cells[i][j];
 }
  } 
  for (int row = 0; row < cells.length ; row++)
  {
    for (int col = 0 ; col < cells[row].length ; col++)
    {
      if ( neighbours(row, col) > 3  ||  neighbours(row, col) < 2 )
      {
        tempCells[row][col] = false;
      }
      else if (neighbours(row, col) == 3 )
      {
        tempCells[row][col] = true;
      }      

    }

  }
  cells = tempCells;

}


public int neighbours (int row, int col) {
  int acc=0;
  for ( int i = row -1; i <= row + 1 ; i++)
    {
     for (int j = col -1 ; j <= col + 1 ; j++)
       {
       try {
         if (cells[i][j]==true && (i != row || j!=col))
         {
           acc++;
         }          
       } catch ( ArrayIndexOutOfBoundsException f)
       {continue;}
     }
  }
  return acc;
}


// Initialises 6 * 6 grid with Glider pattern.
public Life( ) {
final int SIZE = 8;
// Arguably, this should have been a class (static) array.
final int[][] pairs = {{2,4},{3,3},{1,2},{2,2},{3,2}};
cells = new boolean[ SIZE ][ ];
for (int row = 0; row < SIZE; row ++) {
cells[ row ] = new boolean[ SIZE ];
}
for (int pair = 0; pair < pairs.length; pair ++) {
final int row = pairs[ pair ][ 0 ];
final int col = pairs[ pair ][ 1 ];
cells[ row ][ col ] = true;
}
}
 // Initialise size * size grid with random cells.
//public Life( int size ) {
//final Random rand = new Random( );
//cells = new boolean[ size ][ ];
//for (int row = 0; row < size; row ++) {
//cells[ row ] = new boolean[ size ];
//for (int col = 0; col < size; col ++) {
//cells[ row ][ col ] = (rand.nextInt( 2 ) == 0);
//}
//}
//}
// Public methods and helper methods.

@Override
public String toString( ) {
String result = "";
for (int row = 0; row < cells.length; row ++) {
final boolean[] column = cells[ row ];
for (int col = 0; col < column.length; col ++) {
result = result + (column[ col ] ? "x" : ".");
}
result = result + "\n";
}
return result;
}
}
HostileFork says dont trust SE
  • 32,904
  • 11
  • 98
  • 167
user476033
  • 4,607
  • 8
  • 32
  • 35
  • Personally, I would have `tempCells` an instance variable that way you are not allocating a new set of cells for every invocation. You don't need to copy the values however as you are setting them when determining if the cell survives. But seeing as you were not allowed to modify anything else, I guess you were left with no choice. Otherwise looks fine to me. – Jeff Mercado Oct 30 '10 at 10:44
  • @Jeff M: I'd probably have done the same, but strictly that's a (premature) optimization. – Fred Foo Oct 30 '10 at 10:52
  • @user476033 - You deleted your code with a version comment to "keep people from stealing it". I reverted it. Please do not delete information material to (and referenced by) the question. For one thing, it's in the history so people can see it anyway. For a second thing, the whole reason this site exists is to spread information--and the people helping you are doing so with the hope that it will not just "solve your homework" but also help others who visit later with similar questions. If you delete the code it ruins the readability for that purpose. – HostileFork says dont trust SE Sep 24 '11 at 07:24

2 Answers2

4

You don't need to copy the contents of cells to tempCells (the first nested loop in next). Instead, you can add one extra clause to the if-else in the next loop. Also, storing the result from neighbours may be a good idea for both speed and clarity.

for (int row = 0; row < cells.length ; row++)
    for (int col = 0 ; col < cells[row].length ; col++) {
       int n = neighbours(row,col);

       if (n > 3  ||  n < 2)
           tempCells[row][col] = false;
       else if (n == 3)
           tempCells[row][col] = true;
       else
           tempCells[row][col] = cells[row][col];
    }

(Apart from that, looks fine, but I haven't run and tested your code.)

Fred Foo
  • 355,277
  • 75
  • 744
  • 836
  • Thank you so much that makes it much better ! I was worried about the amount of for loops ! :) Wow ! I'm really impressed ! Thank you ! I dont really understand how adding this extra else clause works though, could you please explain it quickly ? It's copying into the array tempCells[row][col] the value that is in cells[row][col]....oooh I get it now ! Thank you ! :) – user476033 Oct 30 '10 at 11:17
0

Don't use ArrayIndexOutOfBoundException to compute out-of-boundary (OOB) conditions. It kills performance. Better use the wrap-around mechanism to treat your array like a sphere so that you don't encounter OOBs at all. You could try something like this:

public Cell[] getNeighbours(int i, int j) {
int i2 = i - 1, i3 = i + 1, j2 = j - 1, j3 = j + 1;
if (i2 == -1) i2 = board.length - 1;
if (i3 == (board.length)) i3 = 0;
if (j2 == -1) j2 = board[i].length - 1;
if (j3 == (board[i].length)) j3 = 0;
return new Cell[]{board[i2][j2], board[i2][j], board[i2][j3], board[i][j2], board[i][j3], board[i3][j2], board[i3][j], board[i3][j3]};

}

Then you can loop through the returned array and check how many of those are alive and return that count.

Tamoghna Chowdhury
  • 208
  • 1
  • 12