HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Feedback on my Conway's Game of Life

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
lifefeedbackgameconway

Problem

I've been programming for about 4 months now, just trying to learn by myself. I've tried my way with coding the Game of Life here, would like some general feedback as well as some pointers on how I can speed it up because right now it seems to run incredibly slowly. Keep in mind I'm a newbie so I would like some easy ways to optimize it.

Both positive and negative feedback are very welcome.

Here's the complete code:

public partial class MainFom : Form
{
    Grid formGrid;
    CancellationTokenSource tokenSrc = new CancellationTokenSource();

    public MainFom()
    {
        InitializeComponent();
    }

    private void MainFom_Load(object sender, EventArgs e)
    {
        formGrid = new Grid();
    }

    private void MainFom_Paint(object sender, PaintEventArgs e)
    {
        e.Graphics.DrawImage(formGrid.toBitmap(), 0, 0);
        e.Graphics.Dispose();
    }

    private void startBtn_Click(object sender, EventArgs e)
    {
        Task tempTask = Task.Factory.StartNew(
            (x) =>
            {
                while (!tokenSrc.IsCancellationRequested)
                {
                    formGrid.UpdateGrid();
                    Graphics graphics = this.CreateGraphics();
                    graphics.Clear(this.BackColor);
                    graphics.DrawImage(formGrid.toBitmap(), 0, 0);
                    graphics.Dispose();
                }
            }, tokenSrc);

        startBtn.Hide();
        Button stopBtn = new Button() { Text = "Stop", Location = startBtn.Location, Size = startBtn.Size };
        this.Controls.Add(stopBtn);
        stopBtn.Click += new EventHandler(
            (x, y) => 
            { 
                tokenSrc.Cancel(); 
                stopBtn.Hide();
                startBtn.Show();
                tempTask.Wait();
                tokenSrc = new CancellationTokenSource();
            });

    }
}


```
class Grid
{
#region Properties/Fields

const int MAX_CELLS_X = 41*2;//41;
const int MAX_CEL

Solution

Some quick comments:

  • Syntax: coord[inate], not "cord".



  • PascalCase is for public properties/fields, local variables should use camelCase: int xCoord = 9001;



I spot a bug

//Create copy of cells since all changes must be done simultaneously
CellCollection copy = cells;


I do not think this does what you think it does.

Cache calculations

Don't redo this 3 times, once for each of the successive ifs: cells.GetNeighbours(cells[i]).Length.

Make CellCollection a Cell[][]

Instead of a List. Because:

  • It would make the structure match better the format of the data;



  • It allows you to find the neighbours directly by index (x-1;y-1, x;y-1, etc) instead of travelling the whoooole list like you are doing - what if the board has 1 million cells?



Outdated comment...

if(RNG.Next(100) < 7)
{ // 10% chance of initial seed creating a live cell


10%? Or 6?

Simplifications

instead of

for (int x = 0; x < MAX_CELLS_X; x++)
{
    int xCoord = 10 * (x + 1);


i'd do

for (int x = 1; x <= MAX_CELLS_X; x++)
{
    int xCoord = 10 * x;


And:

var cell = new Cell(new Rectangle(point, new Size(10, 10)), point);
if (RNG.Next(100) < 7)
{
    cell.IsAlive = true;
}
cells.Add(cell);


or

cells.Add(new Cell(new Rectangle(point, new Size(10, 10)), point) { IsAlive = RNG.Next(100) < 7 });

Code Snippets

//Create copy of cells since all changes must be done simultaneously
CellCollection copy = cells;
if(RNG.Next(100) < 7)
{ // 10% chance of initial seed creating a live cell
for (int x = 0; x < MAX_CELLS_X; x++)
{
    int xCoord = 10 * (x + 1);
for (int x = 1; x <= MAX_CELLS_X; x++)
{
    int xCoord = 10 * x;
var cell = new Cell(new Rectangle(point, new Size(10, 10)), point);
if (RNG.Next(100) < 7)
{
    cell.IsAlive = true;
}
cells.Add(cell);

Context

StackExchange Code Review Q#44327, answer score: 10

Revisions (0)

No revisions yet.