Minimal Game of Life in C#

I want to learn how to write clean code from the start. This is my first project, a 'Minimum Viable Product' implementation of Conway's Game of Life in C#. Mostly I want to know if my code is readable, clean, understandable, etc.

```
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Windows.Forms;

//The universe of the Game of Life is an infinite two-dimensional orthogonal grid of square cells, each of which is in one of two possible states, alive or dead. Every cell interacts with its eight neighbours, which are the cells that are horizontally, vertically, or diagonally adjacent. At each step in time, the following transitions occur:
//Any live cell with fewer than two live neighbours dies, as if caused by under-population.
//Any live cell with two or three live neighbours lives on to the next generation.
//Any live cell with more than three live neighbours dies, as if by overcrowding.
//Any dead cell with exactly three live neighbours becomes a live cell, as if by reproduction.
//The initial pattern constitutes the seed of the system. The first generation is created by applying the above rules simultaneously to every cell in the seed—births and deaths occur simultaneously, and the discrete moment at which this happens is sometimes called a tick (in other words, each generation is a pure function of the preceding one). The rules continue to be applied repeatedly to create further generations.

namespace Life
{
public partial class Form1 : Form
{
public Form1()
{
InitializeComponent();

}
// Some parameters to use throughout the program
// I'll basically use an array to store data and buttons to represent the cells
static int columns = 30; // Columns the Grid will have
static int rows = 20; // Rows the Grid will have
static int depth = 3; // Depth

You defined depth as 3, but, you're only using 2 strings (one for the alive-or-dead, and the second for the neighbours-count).

Instead of storing cell-state as an array of strings, define a class (or perhaps a struct):

enum State
{
    Alive,
    Dead
}
class CellState
{
    internal State State { get; set; }
    internal int Neighbours { get; set; }
    internal CellState() { this.State = State.StateDead; Neighbours = 0; }
}

... also ...

CellState[,] cellGrid = new CellState[columns, rows];

cellGrid[j, i] = new CellState(); // All cells start dead

cellGrid[xIndex, yIndex].State = State.Alive; // Change the cell to "Alive" in the Array

Or (if there are only two states) use a boolean instead of an enum:

class CellState
{
    internal bool IsAlive { get; set; }
    internal int Neighbours { get; set; }
    internal CellState() { this.IsAlive = false; Neighbours = 0; }
}

Creating 400 Button controls becomes expensive. If you had a much larger grid (e.g. 1000x1000) then you (i.e. your machine) couldn't manage it. Instead of Button controls, you could create a Custom Control on which you Paint the cells yourself, and handle its mouse events for hit-tests.

This was confusing at first sight:

for (int i = 0; i < rows; i++)
        {
            for (int j = 0; j < columns; j++)
            {
                int neighbours = 0;

                for (int k = i - 1; k < i + 2; k++)
                {
                    for (int l = j - 1; l < j + 2; l++)

You could name variables:

• i to row

• j to col

• k to i

• l to j

You shouldn't mustn't use exceptions for normal events, for example reaching the edge of the screen.

catch (Exception e) { neighbours += 0; }

Instead don't cause an exception, for example:

for (int k = i - 1; k < i + 2; k++)
{
    if ((k < 0) || (k == rows))
    {
        // beyond edge of screen: not a neighbour
        continue;
    }

Apart from that it's clean, neat, understandable.

There are vertical empty/whitespace lines you should remove.

The string value "Start" exists in more than one method (could be defined as a constant or variable in one place, e.g. in case you want to load it from a multi-lingual resource file).

I was surprised to see you call Stop and Start in your timer1_Tick method: normally a method like that will leave the timer ticking. Perhaps you do it because NexGen takes a long time, so you want to reset the timer in order to see the change before the next tick.

A comment which describes the algorithm being implemented (e.g. a modified version of Conway's Game of Life Rules) could help someone else in the future who needed to maintain your software. They can read the software to see what it does; comments help them know what it's supposed to do (for example in case it's not doing what it's supposed to be doing).

It might be a an idea call Refresh together with SuspendDrawing and ResumeDrawing in your UpdateGrid method. Changing Button instances (causing them to repaint) might (I don't know) be expensive).

You mentioned "MVP": does that mean Model–view–presenter?

If so I'm not seeing the MVP pattern in your code: instead you have one class, with UI events tied to data-state events.

For example, how would you change this (and how much would you need to change) if you wanted to implement Console and WPF versions of this program, as well as the Windows Forms version?


MVP stands for Minimum Viable Product. It's lean startup talk meaning. Something that works with the bare minimum of features and design.

In that case, I suggest the following Minimum changes.

Replace this statement ...

string[, ,] cellGrid = new string[columns, rows, depth]

... with ...

Tuple[,] cellGrid = new Tuple[columns, rows];

That gives you type-safety: use a bool instead of "Alive" and "Dead", and integer expessions instead of expressions like neighbours.ToString() and Convert.ToInt32(cellGrid[xIndex, yIndex, 1]).

Replace this statement ...

for (int k = i - 1; k < i + 2; k++)

... with ...

for (int k = Math.Max(i - 1, 0); k < Math.Min(i + 2, rows); k++)

... and a corresponding change to your l range. Throwing 50 exceptions per calculation is horrendous in my opinion; however I must admit that they're not as bad as I thought they were.

StackExchange Code Review Q#40059, answer score: 11