patterncsharpModerate
Feedback on my Conway's Game of Life
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:
```
class Grid
{
#region Properties/Fields
const int MAX_CELLS_X = 41*2;//41;
const int MAX_CEL
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:
I spot a bug
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:
Make CellCollection a
Instead of a
Outdated comment...
10%? Or 6?
Simplifications
instead of
i'd do
And:
or
- 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 cell10%? 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 cellfor (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.