Problem
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 of the Grid
int cellWidth = 20; // With of the cells which will also be used to determine positions
int cellHeight = 20; // Height of the cells which will also be used to determine position
string[, ,] cellGrid = new string[columns, rows, depth]; // This is the array that will hold the cell's information
Panel Panel1 = new Panel(); // A panel where the cells will be laid out
// Upon Loading the Form
//Add the Panel and Populate with the cells
public void Form1_Load(object sender, EventArgs e)
{
this.Controls.Add(Panel1);
Panel1.Location = new Point(0, 0);
Panel1.Size = new Size(cellWidth * columns, cellHeight * rows);
Panel1.Visible = true;
for (int i = 0; i < rows; i++)
{
for (int j = 0; j < columns; j++)
{
Button cell = new Button();
cellGrid[j, i, 0] = "dead"; // All cells start dead
cell.Location = new Point((j * cellWidth), (i * cellHeight)); // Possition is assigned to cell
cell.Size = new Size(cellWidth, cellHeight);// Size is Assigned to cell
cell.Click += button_Click;
cell.FlatStyle = FlatStyle.Flat; // Style
cell.Margin.All.Equals(0); // Margins
cell.BackColor = Color.White; // Color
Panel1.Controls.Add(cell); // Add to Panel
}
}
}
// When clicking on a cell it will switch between being alive and dead
private void button_Click(object sender, EventArgs e)
{
Button thisButton = ((Button)sender);
// Get the index in cellGrid using the cell's position
int xIndex = thisButton.Location.X / cellWidth;
int yIndex = thisButton.Location.Y / cellHeight;
if (thisButton.BackColor == Color.White) // If the BackColor is white, it means it's dead so
{
thisButton.BackColor = Color.Black; // Change the color to Black
cellGrid[xIndex, yIndex, 0] = "Alive"; // Change the cell to "Alive" in the Array
}
else // Otherwise it's alive so:
{
thisButton.BackColor = Color.White; // Change color to White
cellGrid[xIndex, yIndex, 0] = "Dead"; // Change to Dead in the array
}
}
// This will determine how many Neighbours or live cells each space has
void Neighbours()
{
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++)
{
try
{
if (k == i && l == j) { neighbours += 0; }
else if (cellGrid[l, k, 0] == "Alive") { neighbours += 1; }
}
catch (Exception e) { neighbours += 0; }
}
}
cellGrid[j, i, 1] = neighbours.ToString();
}
}
}
// Switches the grid to the next generation killing and reviving cells following the rules.
public void UpdateGrid()
{
foreach (Control cell in Panel1.Controls)
{
int xIndex = cell.Left / cellWidth;
int yIndex = cell.Top / cellHeight;
int neighbours = Convert.ToInt32(cellGrid[xIndex, yIndex, 1]);
if (neighbours < 2 | neighbours > 3)
{
cellGrid[xIndex, yIndex, 0] = "Dead";
cell.BackColor = Color.White;
}
else if (neighbours == 3)
{
cellGrid[xIndex, yIndex, 0] = "Alive";
cell.BackColor = Color.Black;
}
}
}
// Each generation that passes updates the grid following the rules
public void NextGen()
{
Neighbours();
UpdateGrid();
}
// Each tick of the timer will be a generation
private void timer1_Tick(object sender, EventArgs e)
{
timer1.Stop();
NextGen();
timer1.Start();
}
// When pressing the Start button, generations will start passing automatically
private void StartBtn_Click(object sender, EventArgs e)
{
if (StartBtn.Text == "Start" | StartBtn.Text == "Resume")
{
timer1.Start();
StartBtn.Text = "Pause";
}
else
{
timer1.Stop();
StartBtn.Text = "Resume";
}
}
// Pressing Reset, resets the grid
private void ResetBttn_Click(object sender, EventArgs e)
{
timer1.Stop();
StartBtn.Text = "Start";
for (int i = 0; i < rows; i++)
{
for (int j = 0; j < columns; j++)
{
cellGrid[j, i, 0] = "dead"; // Kill all cells
}
}
NextGen(); //Makes one generation go by
}
// You can pass generations manually by pressing Next Gen Button
private void NextGenBttn_Click(object sender, EventArgs e)
{
NextGen(); //Hace que transcurra una generación
}
// Control how many generations in each second by changing the value
private void GenXSec_ValueChanged(object sender, EventArgs e)
{
timer1.Interval = Convert.ToInt32(1000 / GenXSec.Value);
}
}
}
Solution
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. 1000×1000) 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<bool,int>[,] cellGrid = new Tuple<bool,int>[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.
Going for short’n’sweet, below is my effort (it absolutely won’t win any prizes for performance!). syb0rg has suggested I add a few words regarding how I think this is an improvement. Here goes:
-
Just using 2D integer arrays is representationally lighter.
-
I use abstraction (
Rg,Sum,Doover actions, etc.) to factor
out common patterns. -
I use collections of short functions to do what I want; each function
should be easy to understand by itself. -
By going for brevity, I think I have improved the clarity of the code.
–
void Main()
{
var rows = 10; // Including a 1-cell border o'death.
var cols = 10;
var rnd = new Random();
var cell = new int[rows, cols];
Do(cell, rows, cols, (r, c) => { cell[r, c] = (int)Math.Round(rnd.NextDouble()); });
while (true) {
WriteCells(cell, rows, cols);
Console.WriteLine("--------");
Console.ReadLine();
cell = Gen(cell, rows, cols);
}
}
static IEnumerable<int> Rg = Enumerable.Range(-1, 3);
static int Nbrs(int[,] cell, int r, int c) {
return Rg.Sum(dr => Rg.Sum(dc => cell[r + dr, c + dc]));
}
static int Next(int[,] cell, int r, int c) {
var nbrs = Nbrs(cell, r, c) - cell[r, c];
return (cell[r, c] == 0 && nbrs == 3 || cell[r, c] == 1 && (nbrs == 2 || nbrs == 3)) ? 1 : 0;
}
static void Do(int[,] cell, int rows, int cols, Action<int, int> a) {
for (var r = 1; r < rows - 1; r++) {
for (var c = 1; c < cols - 1; c++) {
a(r, c);
}
}
}
static int[,] Gen(int[,] cell, int rows, int cols) {
var nxt = new int[rows, cols];
Do(cell, rows, cols, (r, c) => { nxt[r, c] = Next(cell, r, c); });
return nxt;
}
static void WriteCells(int[,] cell, int rows, int cols) {
Do(cell, rows, cols, (r, c) => {
Console.Write(cell[r, c] == 0 ? ' ' : '*');
if (c == cols - 2) Console.WriteLine();
});
}
Just a few random notes:
-
Instead of
iandjthe variables could be called asrowNumberandcolumnNumber.Update for Thomas’s comment: Reading (and understanding) code usually takes time but it’s usually not because of long variable or method names. I’d not optimize for character number. There are debates about this topic on Programmer.SE too. My favorite answer is nikie’s one, beacuse of mentioning the short 7-slot (+-2) term memory.
-
cellGrid[j, i, 0]0here is a magic number. A named constant would be better. -
Comments like this are unnecessary:
cell.BackColor = Color.White; // ColorIt says nothing more than the code already does, it’s rather noise. (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)
-
Shorter lines, without horizontal scrolling would be easier to read.
I am known for a bit of the ole’ over engineering but in the interest of separation of concerns I would be inclined to distill the project into it’s core elements.
e.g
internal interface GameOfLifeRenderer
{
GameOfLife GameOfLife { set; }
int CellWidth { get; }
int CellHeight { get; }
void HighlightCell(int x, int y);
void DeHighlightCell(int x, int y);
void Initialize();
void Update();
}
internal interface GameOfLife
{
int Columns { get; }
int Rows { get; }
void Initialize();
void Update();
GridCell GetCell(int x, int y);
}
internal interface GridCell
{
Boolean Alive { get; set; }
GridCell[] GetNeighbours();
}
With that in mind, you could easily have the Form be your View/Renderer and create a presenter to hold it and the GameOfLife object.
Then you would have a nice clean number of decoupled calls, if you then decide down the line to build a non-winform renderer or even just a different render such as a custom graphics object.
Here’s a take on it, that merely does some cleanup of the existing code.
Beyond the changes shown below, I would also highly recommend separating the UI logic from the game logic.
One error I found:
cell.Margin.All.Equals(0); // Margins
doesn’t do anything at all. What I think you mean is:
var margin = cell.Margin;
margin.All = 0;
cell.Margin = margin;
So, without further ado:
namespace Life
{
using System;
using System.Drawing;
using System.Globalization;
using System.Windows.Forms;
/// <summary>
/// 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.
/// </summary>
public partial class Form1 : Form
{
//// Some parameters to use throughout the program
//// I'll basically use an array to store data and buttons to represent the cells
private const string Dead = "Dead";
private const string Alive = "Alive";
/// <summary>
/// Columns the Grid will have
/// </summary>
private const int Columns = 30;
/// <summary>
/// Rows the Grid will have
/// </summary>
private const int Rows = 20;
/// <summary>
/// Depth of the Grid
/// </summary>
private const int Depth = 2;
private const int DeadOrAliveIndex = 0;
private const int NeighborCountIndex = 1;
/// <summary>
/// With of the cells which will also be used to determine positions
/// </summary>
private const int CellWidth = 20;
/// <summary>
/// Height of the cells which will also be used to determine position
/// </summary>
private const int CellHeight = 20;
/// <summary>
/// This is the array that will hold the cell's information
/// </summary>
private readonly string[,,] cellGrid = new string[Columns, Rows, Depth];
/// <summary>
/// A panel where the cells will be laid out
/// </summary>
private readonly Panel panel1 = new Panel();
public Form1()
{
this.InitializeComponent();
}
// Upon Loading the Form
// Add the Panel and Populate with the cells
public void Form1_Load(object sender, EventArgs e)
{
this.Controls.Add(this.panel1);
this.panel1.Location = new Point(0, 0);
this.panel1.Size = new Size(CellWidth * Columns, CellHeight * Rows);
this.panel1.Visible = true;
for (var row = 0; row < Rows; row++)
{
for (var column = 0; column < Columns; column++)
{
var cell = new Button();
this.cellGrid[column, row, DeadOrAliveIndex] = Dead;
cell.Location = new Point(CellWidth * column, CellHeight * row);
cell.Size = new Size(CellWidth, CellHeight);
cell.Click += this.ButtonClick;
cell.FlatStyle = FlatStyle.Flat;
var margin = cell.Margin;
margin.All = 0;
cell.Margin = margin;
cell.BackColor = Color.White;
this.panel1.Controls.Add(cell);
}
}
}
// When clicking on a cell it will switch between being alive and dead
private void ButtonClick(object sender, EventArgs e)
{
var thisButton = sender as Button;
if (thisButton == null)
{
return;
}
// Get the index in cellGrid using the cell's position
var row = thisButton.Location.Y / CellHeight;
var column = thisButton.Location.X / CellWidth;
// If the BackColor is white, it means it's dead so
var isDead = thisButton.BackColor == Color.White;
thisButton.BackColor = isDead ? Color.Black : Color.White;
this.cellGrid[column, row, DeadOrAliveIndex] = isDead ? Alive : Dead;
}
// This will determine how many Neighbours or live cells each space has
private void Neighbours()
{
for (var row = 0; row < Rows; row++)
{
for (var column = 0; column < Columns; column++)
{
var neighbours = 0;
for (var i = row - 1; i < row + 2; i++)
{
if (i < 0 || i >= Rows)
{
continue;
}
for (var j = column - 1; j < column + 2; j++)
{
if (j < 0 || j >= Columns)
{
continue;
}
if (this.cellGrid[j, i, DeadOrAliveIndex] == Alive)
{
neighbours++;
}
}
}
this.cellGrid[column, row, NeighborCountIndex] = neighbours.ToString(CultureInfo.InvariantCulture);
}
}
}
// Switches the grid to the next generation killing and reviving cells following the rules.
public void UpdateGrid()
{
foreach (Control cell in this.panel1.Controls)
{
var row = cell.Top / CellHeight;
var column = cell.Left / CellWidth;
var neighbours = Convert.ToInt32(this.cellGrid[column, row, NeighborCountIndex]);
if (neighbours < 2 || neighbours > 3)
{
this.cellGrid[column, row, DeadOrAliveIndex] = Dead;
cell.BackColor = Color.White;
}
else if (neighbours == 3)
{
this.cellGrid[column, row, DeadOrAliveIndex] = Alive;
cell.BackColor = Color.Black;
}
}
}
// Each generation that passes updates the grid following the rules
public void NextGen()
{
this.Neighbours();
this.UpdateGrid();
}
// Each tick of the timer will be a generation
private void timer1_Tick(object sender, EventArgs e)
{
this.timer1.Stop();
this.NextGen();
this.timer1.Start();
}
// When pressing the Start button, generations will start passing automatically
private void StartBtn_Click(object sender, EventArgs e)
{
if (this.StartBtn.Text == "Start" || this.StartBtn.Text == "Resume")
{
this.timer1.Start();
this.StartBtn.Text = "Pause";
}
else
{
this.timer1.Stop();
this.StartBtn.Text = "Resume";
}
}
// Pressing Reset, resets the grid
private void ResetBttn_Click(object sender, EventArgs e)
{
this.timer1.Stop();
this.StartBtn.Text = "Start";
// Kill all cells
for (var row = 0; row < Rows; row++)
{
for (var column = 0; column < Columns; column++)
{
this.cellGrid[column, row, DeadOrAliveIndex] = Dead;
}
}
this.NextGen(); // Makes one generation go by
}
// You can pass generations manually by pressing Next Gen Button
private void NextGenBttn_Click(object sender, EventArgs e)
{
this.NextGen(); // Hace que transcurra una generación
}
// Control how many generations in each second by changing the value
private void GenXSec_ValueChanged(object sender, EventArgs e)
{
this.timer1.Interval = Convert.ToInt32(1000 / this.GenXSec.Value);
}
}
}