'Game of Life' in C#

Question

Unrelated to code:

I have only been programming for three weeks, and this is my first project.

About the code:

1. Uses a preset grid.

2. It was all done on a single page. This, I realise, was a mistake, and I have fixed that since.

3. I used an int array for the grid. I did first think maybe a Boolean array would be better, but for my skill level int was easier. I do plan to try a bool array as soon as this int version is completed.

4. I tried to find another way to implement the logic for the game using a for a,b loop instead of a lot of if statements, but I was unsuccessful. Is it possible to do it this way?

5. I understand my variable naming is poor and could be greatly improved. I do plan to make changes to them in the near future.

6. I know use of the endgame variable is unnecessary, but I kept it for use in enhancements I plan to make to the project.

What I would like from this:

1. A pointer on how to improve the change from one grid to another, i.e. to improve the flow of the game.

2. Thoughts on how to improve the logic of my grid, by avoiding overuse of conditional statements.

3. General feedback to code both structurally and aesthetically, so I know what to improve in future projects.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace grid
{
    class Program
    {
        static void Main(string[] args)
        {
                bool endgame = false;
                int[,] grid = new int[,]
        {
        { 1, 0, 0, 0, 0, 0, 0, 0, 1, 1,},
        { 0, 0, 1, 0, 0, 1, 0, 0, 0, 0,},
        { 0, 1, 1, 0, 1, 1, 1, 0, 0, 0,},
        { 0, 1, 0, 0, 0, 1, 0, 0, 0, 0,},
        { 0, 0, 0, 0, 0, 0, 0, 1, 0, 1,},
        { 0, 0, 0, 0, 1, 0, 0, 0, 1, 1,},
        { 0, 1, 0, 0, 1, 0, 0, 1, 0, 0,},
        { 1, 1, 0, 0, 0, 0, 0, 0, 0, 0,},
        { 1, 1, 0, 0, 1, 0, 0, 1, 0, 0,},
        { 1, 1, 0, 0, 0, 0, 0, 0, 0, 1,}
        };
                gridconstructor gameoflife = new gridconstructor();
                while (endgame == false)
                {
                    Console.WriteLine("Welcome to my Game of life simulator!");
                    Console.WriteLine("To begin enter 1 , to quit enter 2:");
                    Console.WriteLine();
                    int choice = Convert.ToInt32(Console.ReadLine());
                    if (choice == 1)
                    {
                        gameoflife.activegird(grid);
                    }
                    else
                    {
                        Environment.Exit(1);
                    }
                }
        }

    }

    class gridconstructor
    {
        int[,] livegrid;
        int[,] oldgrid;

        // Method to carry out grid copy and checks
        public void activegird(int[,] grid)
        {
            // Copy of grid
            livegrid = (int[,])grid.Clone();

            // Get number of rows/columns in grid
            int gridcol = livegrid.GetLength(0);
            int gridrow = livegrid.GetLength(1);

            // Nested loop to itterate through grid
            for (int i = 0; i < gridcol; i++)
            {
                for (int j = 0; j < gridrow; j++)
                {
                    // get number live cells around position on grid
                    int livecellcount = cellchecker(i,j);

                    // if already live logic
                    if (livegrid[i,j] == 1)
                    {
                        if (livecellcount <= 1)
                        {
                            livegrid[i, j] = 0;
                        }
                        else if (livecellcount == 2 || livecellcount == 3)
                        {
                            livegrid[i, j] = 1;
                        }
                        else
                        {
                            livegrid[i, j] = 0;
                        }
                    }
                    // if dead logic
                    else
                    {
                        if (livecellcount == 3)
                        {
                            livegrid[i, j] = 1;
                        }
                        else
                        {
                            livegrid[i, j] = 0;
                        }
                    }
                }
            }
            // Calling method to display changes and repeat cycle.
            oldgrid = (int[,])livegrid.Clone();
            displaygrid(livegrid);
            Console.WriteLine("If you wish to continue enter y: ");
            string checkkey = Console.ReadLine();
            checkkey = checkkey.ToLower();
            if (checkkey == "y")
            {
                activegird(oldgrid);
            }

            else
            {
                return;
            }

        }



        // Method to check surrounding cells of a position on grid
        public int cellchecker(int x, int y)
        {

            // int var to hold number live cells 
            int livecells = 0;

            // checks +1 and +1
            if (x < 9 && y < 9)
            {
                if (livegrid[x + 1, y + 1] == 1)
                {
                    livecells++;
                }
            }

            // checks +1 and 0
            if (x < 9)
            {
                if (livegrid[x + 1, y] == 1)
                {
                    livecells++;
                }
            }
            // checks 0 and +1
            if (y<9)
            {
                if (livegrid[x, y + 1] == 1)
                {
                    livecells++;
                }

            }
            // checks -1 and -1
            if (x > 0 && y > 0)
            {
                if (livegrid[x - 1, y - 1] == 1)
                {
                    livecells++;
                }
            }
            // checks 0 and -1
            if (y>0)
            {
                if (livegrid[x, y - 1] == 1)
                {
                    livecells++;
                }
            }
            // checks -1 and 0
            if (x>0)
            {
                if (livegrid[x - 1, y] == 1)
                {
                    livecells++;
                }
            }
            // checks 1 and -1
            if (x < 9 && y > 0)
            {
                if (livegrid[x + 1, y - 1] == 1)
               {
                    livecells++;
               }
            }
            // checks -1 and 1
            if (x > 0 && y < 9)
            {
                if (livegrid[x - 1, y + 1] == 1)
                {
                    livecells++;
                }
            }
            // returns total number of live cells
            return livecells;

        }

        public void displaygrid(int[,] livegrid)
        {
            int gridcol = livegrid.GetLength(0);
            int gridrow = livegrid.GetLength(1);

            for (int i = 0; i < gridcol; i++)
            {
                for (int j = 0; j < gridrow; j++)
                {
                    Console.Write("{0,2}", livegrid[i, j]);
                }
                Console.WriteLine();
            }
        }
    }
} 

Answer 1

This is very important: Your algorithm is wrong. Do not ask for help with style or efficiency or anything else like that until you get the algorithm right! Hint: you should not be updating the grid at the same time as you are reading from it.

Get the code compiling, then get it right, and then get it elegant and then get it fast. Always that order.

So let's start by getting the code right by identifying where it is wrong. It's wrong here:

           int livecellcount = cellchecker(i,j);
           if (livegrid[i,j] == 1)
           {
             if (livecellcount <= 1)
               livegrid[i, j] = 0;

Suppose cell i, j is 1 and the count of live neighbours is 1; specifically, the next cell over is the only live cell. Now suppose that next cell over has 2 live neighbours. What must happen when you compute the state of the next cell? It is live and it has two live neighbours, so it must stay alive. What happens in your program? You set the current cell to dead, and then you count the number of live neighbors of the next cell, which will then be 1, not 2, and the cell will die!

You need to be doing the reading on the previous grid, and the writing on the next grid, in order for this to be correct.

There is a mechanism for making sure you never make this terrible mistake again, which I shall now describe.

To address your specific questions:

A pointer on how to improve the change from one grid to another, i.e. to improve the flow of the game.

What I would like to see is first, a class that represents the grid, rather than an array. And second, that class should be immutable. A grid is not something you change. The next step in the game is a new grid. There should be a method somewhere with a signature like:

static Grid Next(Grid g)

or an instance method on the Grid class that returns a new Grid. Never change an existing grid. If you never change an existing grid and only construct new grids, then you can never make the mistake of mutating the wrong grid.

Also, you don't need to clone the existing grid. The existing grid never changes, and every cell of the new grid will be set to the correct value, so why would you need to clone the grid? Read the old grid, write the new grid, always.

Thoughts on how to improve the logic of my grid, by avoiding overuse of conditional statements.

Let's take a look at some of your logic:

if (livecellcount <= 1)
  livegrid[i, j] = 0;
else if (livecellcount == 2 || livecellcount == 3)
  livegrid[i, j] = 1;
else
  livegrid[i, j] = 0;

We immediately see that the first clause is completely unnecessary; this has identical results:

if (livecellcount == 2 || livecellcount == 3)
  livegrid[i, j] = 1;
else
  livegrid[i, j] = 0;

So now let's take a look at that in context. I'll fix the bug while I'm at it:

if (oldGrid[i,j] == 1) // Always read the ORIGINAL grid.
{
  if (livecellcount == 2 || livecellcount == 3)
    newGrid[i, j] = 1;
  else
    newGrid[i, j] = 0;
}
else
{
  if (livecellcount == 3)
    newGrid[i, j] = 1;
  else
    newGrid[i, j] = 0;
}

Can we simplify this further? Yes. We begin by noticing that the "3" condition is repeated on both branches of the outer "if", and so can be brought outside:

if (livecellcount == 3)
  newGrid[i, j] = 1;
else if (oldGrid[i,j] == 1)
{
  if (livecellcount == 2)
    newGrid[i, j] = 1;
  else
    newGrid[i, j] = 0;
}
else
  newGrid[i, j] = 0;

Can we simplify this further? There are now only two places where the new cell is live, so let's combine those two conditions:

if (livecellcount == 3 || oldGrid[i, j] == 1 && livecellcount == 2)
  newGrid[i, j] = 1;
else
  newGrid[i, j] = 0;

This is already orders of magnitude shorter. Can we make it better still? Sure! We notice that we have four numbers: 0, 1, 2 and 3. But only 2 and 3 are numbers. The 0 and 1 are alive and dead. So let's say so:

const int Alive = 1;
const int Dead = 0;
...
if (livecellcount == 3 || oldGrid[i, j] == Alive && livecellcount == 2)
  newGrid[i, j] = Alive;
else
  newGrid[i, j] = Dead;

So much easier to read. Now the numbers are actually numbers and the special values are named with their meanings.

Can we make it better? Yes. Abstract policies into methods:

static int Policy(int cell, int liveNeighbors)
{
  if (liveNeighbors == 3 || cell == Alive && liveNeighbors == 2)
    return Alive;
  return Dead;
}
...
newGrid[i, j] = Policy(oldGrid[i, j], liveNeighbors);

OMG look at how much nicer that code is compared to what you wrote. You can just read the code practically like English: the policy is that if the number of live neighbours is three, or the cell is alive and the number of live neighbours is two, then the new cell is alive. Otherwise it is dead.

General feedback to code both structurally and aesthetically, so I know what to improve in future projects.

• Right is better than elegant. Get the code right first.
• Strive to make your code read more like the ideas it is trying to represent. Don't use 0 and 1; use Alive and Dead.
• Got a special rule? Abstract it away into a method that implements that rule.
• Got some data with interesting properties? Abstract it into a class. Remember, coding is about making good abstractions.
• Fix the spelling mistakes. "gird" makes my eyes hurt.
• Use standard naming conventions.
• Embrace immutable style; as we've just seen, most bugs happen when something changes in the wrong order.
• Test things! You would have probably found your bug if you had written a test case that took a known grid, ran it forward a generation, and then compared that to the result that you had worked out by hand.

Answer 2

Cell

The first thing I would do is actually not switch to a bool grid but actually make a class that represents a single cell. I'm going to call it Cell because I'm not that creative.

The cell would handle the old and new flags to start with but it would also let you easily add logic for age (some GOL implementations color the cells based on their age).

Cell would not be aware of anything else.

This will let you encapsulate the logic of the Cell in a single place.

Grid

Next, I would have a class that handles just the grid (look up Single Responsibility Principle for more reasons why). At the base level, it can easily take just a width and height variable and creates an array (of your choice) of Cell objects.

You could have Cell GetCell(x, y) that retrieves a cell at a given location. I'd also write a function List<Cell> GetAdjacentCells(x, y) to retrieve the list of cells next to the given coordinates. For GOL, that's all you really need.

Since Grid know its height and width, you don't have to get the GetLength() unless you want to.

GridUpdater

If I was writing it, I would have a separate class for updating the state. You could do this within Grid itself, but it would easier to maintain and test if you did it separately (again, see SRP).

Breaking apart the Console stuff out of there also keeps your code relatively clean (and SRP).

With GOL, you basically have two sweeps. One is to calculate the new values. The second is to move the new value into the current one.

for (int x = 0; x < Width; x++) {
  for (int y = 0; y < Height; y++) {
    var cell = grid.GetCell(x, y);
    var adjacent = grid.GetAdjacentCells(x, y);
    cell.Calculate(adjacent);
  }
}

for (int x = 0; x < Width; x++) {
  for (int y = 0; y < Height; y++) {
    var cell = grid.GetCell(x, y);
    cell.Update(); // Should be a function for encapsulation.
  }
}

The reason I suggested the logic for cell is because the Calculate function there is pretty simple.

public void Calculate(List<Cell> adjacent)
{
  // All the rules are based on the number of adjacent cells.
  var count = adjacent.Count;

  // Less than two or greater than three is always dead.
  if (count < 2 || count > 3) {
    NewState = false;
  } else {
    // For live (OldState = true) cells, they are alive. For dead ones,
    // they live only if there is exactly three. This uses the OR logic to
    // to combine the two statements together.
    NewState = OldState || count == 3;
  }
}

You don't need much more than this. Your Console stuff should be in a fourth class because it has nothing to do with the state of a cell, the size of the grid, or how the grid is updated.

Formatting

Typically C# uses PascalCase for all method and property names.

while (endgame == false) is while (!endgame).

Comprehension wise, it is great if your method names used a verb in the name. So, instead of activecell, you use CalculateNewState or Update. That way, someone new can understand what that function is supposed to do. "Active" is really just an adjective on "Cell" which is a noun.

EDIT: NewState = false; to say dead.

Answer 3

With code like this

// if dead logic
else
{
    if (livecellcount == 3)
    {
        livegrid[i, j] = 1;
    }
    else
    {
        livegrid[i, j] = 0;
    }
}

You could make it a single one line inside of the else block by using what is called a ternary statement.

A ternary statement is structured like this

variableName = Conditional ? if true do this : if false do this;

so your code would look like this

livegrid[i,j] = livecellcount == 3 ? 1 : 0;

Another thing that you should keep in mind is C#'s naming standards

as you can see the classes are PascalCase Program but the class that you created is not PascalCase.

• Classes and Methods are PascalCase
• Variables are camelCase
• Constants are also camelCase

_{personally I prefer SHOUTCASE for Constants, but I think it is just a matter of preference for the developer/team}

What ever style you (or your team) go with, make sure to stay consistent.

Answer 4

Another thing: you could also make this code a lot easier to read:

        if (x < 9 && y < 9)
            if (livegrid[x + 1, y + 1] == 1)
                livecells++;
        if (x < 9)
            if (livegrid[x + 1, y] == 1)
                livecells++;
            if (livegrid[x, y + 1] == 1)
                livecells++;
        if (x > 0 && y > 0)
            if (livegrid[x - 1, y - 1] == 1)
                livecells++;
        if (y>0)
            if (livegrid[x, y - 1] == 1)
                livecells++;
        if (x>0)
            if (livegrid[x - 1, y] == 1)
                livecells++;
        if (x < 9 && y > 0)
            if (livegrid[x + 1, y - 1] == 1)
                livecells++;
        if (x > 0 && y < 9)
            if (livegrid[x - 1, y + 1] == 1)
                livecells++;
        return livecells;

How? Break it up!

const XSize = 9;
const YSize = 9;
bool ValidX(int x) => 0 <= x && x < XSize;
bool ValidY(int y) => 0 <= y && y < YSize;

Note that this uses a new syntax for C# 6; in C# 5 you'd say

bool ValidX(int x)
{
  return 0 <= x && x < XSize;
}

The new syntax is just shorter for small functions.

Now we can say:

int ValidOrDead(int x, int y) => ValidX(x) && ValidY(y) ? grid[x, y] : Dead;
int North(int x, int y) => ValidOrDead(x, y-1);
int South(int x, int y) => ValidOrDead(x, y+1);
int West(int x, y) => ValidOrDead(x-1, y);

and so on. So now we can say:

int LiveNeighbors(int x, int y) => 
  North(x, y) + South(x, y) + West(x, y) + ...

Again look at how much easier it is to read code when you break it up into tiny methods, each of which does one thing correctly.