Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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);
        }


    }
}
share|improve this question
3  
MVP? Wait, what? What does that stand for in this context? I don't think you're trying to figure out who was the most valuable player in the Game of Life... –  user2357112 Jan 26 at 2:28
    
@user2357112 I don't understand the use of MVP either. –  Thomas Jan 26 at 3:17
    
Thanks for all the feedback. MVP stands for Minimum Viable Product. It's lean startup talk meaning. Something that works with the bare minimum of features and design –  Santiago Gil Jan 26 at 4:55
5  
@user2900210 You might want to define such non-standard acronyms in your question body in the future, not everybody speaks lean-startup :) –  Thomas Jan 26 at 5:21
add comment

5 Answers

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<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.

share|improve this answer
    
Thanks for all the feedback. MVP stands for Minimum Viable Product. It's lean startup talk meaning. Something that works with the bare minimum of features and design. –  Santiago Gil Jan 26 at 4:54
1  
Why did you mark the members of CellState as internal? If the whole class is already internal (which is the default), I think it makes sense to make the members public. That way, if you decide to make the class public later on, you don't need to change all the members. –  svick Jan 26 at 10:16
    
@svick I think the default accessibility for classes (i.e. for the name of the class) is internal, but the default accessibility for the class's members (methods, properties, and fields) is private. I made CellState properties and constructor internal so that they would be visible to the Form1 class. –  ChrisW Jan 26 at 10:27
    
@ChrisW Yes, what I was trying to say is that you should have made the members public instead. It would still behave the same now, and it would be more future-proof. –  svick Jan 26 at 10:29
    
@svick You are right. I'm in the habit of defining everything, where they're not private or protected, as internal. So usually, for me, public is a signal that it's a method which implements some underlying interface. If I needed to justify that, I'd say it's to communicate (to other programmers) the programmer's intent: a public method in an internal class looks strange ("why did he declare it public when the class is internal?"). –  ChrisW Jan 26 at 10:49
add comment

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, Do over 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();
    });
}
share|improve this answer
    
@syb0rg, fair enough. Sometimes short code can speak for itself; here I have tried to demonstrate clarity and brevity by omitting unnecessary fluff. –  Rafe Feb 4 at 2:40
    
@syb0rg, Righto: my representation is lighter and, through abstraction (Rg, Sum, Do over actions), my code is shorter, more regular, and hence more comprehensible. I've completely skipped the usual OO chaff (classes, interfaces, inheritance, design patterns...) because it really doesn't help solve the problem. –  Rafe Feb 4 at 3:18
    
Here ya go! Vote reversed! :) –  Mat's Mug Feb 4 at 3:45
add comment

Just a few random notes:

  1. Instead of i and j the variables could be called as rowNumber and columnNumber.

    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.

  2. cellGrid[j, i, 0]
    

    0 here is a magic number. A named constant would be better.

  3. Comments like this are unnecessary:

    cell.BackColor = Color.White; // Color
    

    It says nothing more than the code already does, it's rather noise. (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)

  4. Shorter lines, without horizontal scrolling would be easier to read.

share|improve this answer
    
Imho your first advice is a bit too verbose. i and j are fairly standard index variable names, as long as you define them properly. row and col maybe, but rowNumber and columnNumber is just way too long. –  Thomas Jan 26 at 9:34
    
@Thomas: I'v updated the answer a little bit about that. –  palacsint Jan 26 at 10:13
1  
Typos like for (int j = 0; i < columns; j++) are easy to make, and hard to spot. If you mistype column it's a bright red line. –  abuzittin gillifirca Jan 27 at 12:30
    
@palacsint Sure, but the type (int) already indicates that they are numbers. So in that respect rowNumber and columnNumber are rather redundant names, so row and column are more appropriate IMHO. Being explicit doesn't mean being redundant. –  Thomas Mar 1 at 9:00
1  
@Thomas: When I wrote them I thought of them as rowIndex and coluumnIndex I guess it would have been better, thanks for pointing that out! (An integer also could be a counter, a minimal value etc., so the index postfix does not seem redundant to me.) –  palacsint Mar 1 at 11:37
add comment

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);
        }
    }
}
share|improve this answer
add comment

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.

share|improve this answer
    
Why interfaces instead of classes? Why is HighlightCell a method of Renderer not of Game? Why GetNeighbours instead of CountLiveNeighbours? Where do you store the 'temporary' neighbour count or the pending Alive value, and implement the game rules? –  ChrisW Jan 27 at 13:52
    
@ChrisW I just did I high level overview, not the full implementation. There would of course be a backing class file behind each of these. The point of the interfaces is to show only the information that should be public. The actual implementation such as temporary values and calculations should not be exposed in case they change –  apieceoffruit Jan 27 at 13:56
    
So GameOfLife is more like GridCellArray, containing GridCell instances but no rules? What do Initialize and Update do? –  ChrisW Jan 27 at 13:59
    
@ChrisW HighlightCell is a rendering task. So it goes in the renderer. Game on the other hand probably should have a SelectCell function. If you are interesed, When I get home from work I could put together a full implementation, although I'll post it as a secondary question so the other fine fellows here can correct my implementation as the question is slightly different from yours. Yours was in respect to a minimalist solution while what I outlined above is a more over-engineered extensible solution. –  apieceoffruit Jan 27 at 14:04
    
@ChrisW the idea is to think of what is the simplest thing each part needs to know? renderer doesn't need to know rules, rules and gridSquare don't need to know how it will be displayed. Once you have separated out each section you can theoretically swap them out. for example you could have a GameOfLifeStandardRuleSet,GameOfLifeZombieRuleSet etc and it will still be able to display the same regardless, or have a GameOfLifeWinformRenderer,GameOfLifeWPFRenderer,GameOfLifeConsoleRenderer etc –  apieceoffruit Jan 27 at 14:09
show 1 more comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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