3
\$\begingroup\$

This is a working Neighbours method. Is there any way to make this simpler and concise?

public int Neighbours(int xCoordinate, int yCoordinate)
        {
            xCoordinate = xCoordinate -1;
            yCoordinate = yCoordinate -1;
            int NeighbourCounter =0;
            int incrementer = 0;
            int i1 =-1; 
            int j1 =-1;
            int i2 =1; 
            int j2 =1; 

            if(yCoordinate==0)
            {
                j1=1;
            }

            if(xCoordinate==0)
            {
                i1=1;
            }

            if(yCoordinate+1==xLen)
            {
                j2=0;
            }

            if(xCoordinate+1==yLen)
            {
                i2=0;
            }

            for(int j=j1; j<=j2; j++)
            {
                if(j==0)
                {
                    incrementer = 2;
                }
                else
                {
                    incrementer = 1;
                }

                for(int i=i1; i<=i2; i+=incrementer)
                {
                    if(CG[yCoordinate + j][xCoordinate + i] == 1)
                    {
                        NeighbourCounter++;
                    }
                }
            }
                return NeighbourCounter;
        }
\$\endgroup\$
6
\$\begingroup\$

There are several style issues with this code. Most notably:

  • Use camelCase for method names
  • Use camelCase for variable names
  • Put spaces around operators, for example:
    • i = i1 instead of i=i1
    • yCoordinate + 1 == xLen instead of yCoordinate+1==xLen
    • ... and so on
  • j1, j2, ... are poor variable names

To simplify this code, a couple of ideas:

  • Use an enum to represent neighbors: North, NorthEast, East, SouthEast, ...
    • Create custom fields dx and dy to represent the direction offset
  • Iterate over Neighbor.values(), for each item perform this check:
    • Calculate the neighbor position using current position + the neighbor's dx and dy
    • Check if the position is on the grid, for example with a isValid(Position pos, Neighbor neighbor) method
    • Check if the position is alive, for example with a isAlive(Position pos, Neighbor neighbor) method
    • If both the above are true, increment the count

To get you started, here's an example enum:

enum Neighbor {
    North(0, 1),
    NorthEast(1, 1),
    East(1, 0);

    private final int dx;
    private final int dy;

    Neighbor(int dx, int dy) {
        this.dx = dx;
        this.dy = dy;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thank you, I was just testing to see if I could get it to work; Also i am not familiar with the enum method as we haven't learnt that \$\endgroup\$ – RandomMath Nov 25 '14 at 23:06
0
\$\begingroup\$

You are asking for a more simple and more readable solution. How about this

public int Neighbours(int xCoordinate, int yCoordinate)
{
  int NeighbourCounter = 0;

  for (int x = xCoordinate - 1; x <= xCoordinate + 1; x++)
    if (x >= 0 && x <= xLen)
      for (int y = yCoordinate - 1; y <= yCoordinate + 1; y++)
        if (y >= 0 && y <= yLen)
          if (x != xCoordinate || x != yCoordinate)
             if (CG[x,y] == 1) 
               NeighbourCounter++;

  return NeighbourCounter; 
}

I left out parenthes as they would blow up the code. You could add them, although they are redundant. It is a matter of taste.

This is the most straightforward attempt. Simply loop through all eight neighbors, test if the neighbors are within the field. If they are, count existing cells, but ignore the own position.

There are some optimizations possible. Although it is hard to optimize a loop over max 8 elements. The most obvious improvement would be to eliminate the test for the boundaries. This can be done by increasing the board by one row and col at each side.

You are not allowed to put anything there, but testing would be greatly simplified. So valid positions would be [1.. xLen-1] and [1...yLen-1].

The simplified code looks like this (notice the removed boundary checks)

public int Neighbours(int xCoordinate, int yCoordinate)
{
  int NeighbourCounter = 0;

  for (int x = xCoordinate - 1; x <= xCoordinate + 1; x++)
    for (int y = yCoordinate - 1; y <= yCoordinate + 1; y++)
        if (x != xCoordinate || x != yCoordinate)
           if (CG[x,1] == 1) 
             NeighbourCounter++;

  return NeighbourCounter; 
}

There are two more lines which we could optimize: The 'if' conditions. Considering the fact that the board consists of '0' and '1', we could do the following 1. Add whatever we find in the cell (0 or 1) 2. Count the center cell but later substract whatever it contains.

public int Neighbours(int xCoordinate, int yCoordinate)
{
  int NeighbourCounter = 0;

  for (int x = xCoordinate - 1; x <= xCoordinate + 1; x++)
    for (int y = yCoordinate - 1; y <= yCoordinate + 1; y++)
       NeighbourCounter += CG[x,y];

  return NeighbourCounter - CG[xCoordinate, yCoordinate]; 
}

In my point of view, it cannot get simpler than that. However, if you ask for ease of understanding, I would not use this code as you have to explain the code (add the contents of the cell?) and the last line ('return'). Also, when changing the board layout (e.g. from 0/1 to 0,99 or true/false) the code will not work like that anymore.

However, it is the most compact.

Now, when it comes to performance improvement you might consider maintaining a second matrix, which actually contains the current count of neightbors for each cell. This matrix is updated whenever you set or reset a cell.

Setting a cell involves adding 1 to all neighbors while resetting a cell involves substracting 1 from all neighbors.

void SetCell (int x, int y)
{
  if (CG[x,y] == 1) return;
  CG[x,y] = 1;
  for (int x0 = x-1; x0 <= x+1; x0++)
    for (int y0 = y-1; y0 <= y+1; y0++)    
      Neighbors[x0,y0]++;

  Neighbors[x,y]--; // We are not our own neighbor
}

void SetCell (int x, int y)
{
  if (CG[x,y] == 1) return;
  CG[x,y] = 1;
  for (int x0 = x-1; x0 <= x+1; x0++)
    for (int y0 = y-1; y0 <= y+1; y0++)    
      Neighbors[x0,y0]--;

  Neighbors[x,y]++; // We are not our own neighbor
}

There are more cells on the board than cells that are set or reset at each round, hence the above loops are not called for each cell on the board but only for the few that are modified. I am quite sure this is a great performance improvement.

\$\endgroup\$
  • \$\begingroup\$ i tried using that code however i keep getting an ArrayOutOfBoundsException \$\endgroup\$ – RandomMath Nov 26 '14 at 15:45
  • \$\begingroup\$ As janos suggested please use lowercase for the first letter of method and variable names. CG is a bad name two-fold: 1. ALL UPPERCASE is reserved for final variables (a.k.a. constants), 2. using abbreviated names like this is not very descriptive. \$\endgroup\$ – Gerold Broser Nov 28 '14 at 22:38
  • \$\begingroup\$ The original question was "Is there any way to make this simpler and concise?" It did not mention 'prettier'. So I followed his nomenclature. @RandomMath: Please check the bounds of your array. I did not test the code I posted but tried to show you possible ways to optimize and simplify. Please check yourself: It's the best way to learn. \$\endgroup\$ – alzaimar Nov 29 '14 at 9:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.