13
\$\begingroup\$

Unrelated to code:

I have only been programming for three weeks, and this is my first project.

About the code:

  1. Uses a preset grid.

  2. It was all done on a single page. This, I realise, was a mistake, and I have fixed that since.

  3. I used an int array for the grid. I did first think maybe a Boolean array would be better, but for my skill level int was easier. I do plan to try a bool array as soon as this int version is completed.

  4. I tried to find another way to implement the logic for the game using a for a,b loop instead of a lot of if statements, but I was unsuccessful. Is it possible to do it this way?

  5. I understand my variable naming is poor and could be greatly improved. I do plan to make changes to them in the near future.

  6. I know use of the endgame variable is unnecessary, but I kept it for use in enhancements I plan to make to the project.

What I would like from this:

  1. A pointer on how to improve the change from one grid to another, i.e. to improve the flow of the game.

  2. Thoughts on how to improve the logic of my grid, by avoiding overuse of conditional statements.

  3. General feedback to code both structurally and aesthetically, so I know what to improve in future projects.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace grid
{
    class Program
    {
        static void Main(string[] args)
        {
                bool endgame = false;
                int[,] grid = new int[,]
        {
        { 1, 0, 0, 0, 0, 0, 0, 0, 1, 1,},
        { 0, 0, 1, 0, 0, 1, 0, 0, 0, 0,},
        { 0, 1, 1, 0, 1, 1, 1, 0, 0, 0,},
        { 0, 1, 0, 0, 0, 1, 0, 0, 0, 0,},
        { 0, 0, 0, 0, 0, 0, 0, 1, 0, 1,},
        { 0, 0, 0, 0, 1, 0, 0, 0, 1, 1,},
        { 0, 1, 0, 0, 1, 0, 0, 1, 0, 0,},
        { 1, 1, 0, 0, 0, 0, 0, 0, 0, 0,},
        { 1, 1, 0, 0, 1, 0, 0, 1, 0, 0,},
        { 1, 1, 0, 0, 0, 0, 0, 0, 0, 1,}
        };
                gridconstructor gameoflife = new gridconstructor();
                while (endgame == false)
                {
                    Console.WriteLine("Welcome to my Game of life simulator!");
                    Console.WriteLine("To begin enter 1 , to quit enter 2:");
                    Console.WriteLine();
                    int choice = Convert.ToInt32(Console.ReadLine());
                    if (choice == 1)
                    {
                        gameoflife.activegird(grid);
                    }
                    else
                    {
                        Environment.Exit(1);
                    }
                }
        }

    }

    class gridconstructor
    {
        int[,] livegrid;
        int[,] oldgrid;

        // Method to carry out grid copy and checks
        public void activegird(int[,] grid)
        {
            // Copy of grid
            livegrid = (int[,])grid.Clone();

            // Get number of rows/columns in grid
            int gridcol = livegrid.GetLength(0);
            int gridrow = livegrid.GetLength(1);

            // Nested loop to itterate through grid
            for (int i = 0; i < gridcol; i++)
            {
                for (int j = 0; j < gridrow; j++)
                {
                    // get number live cells around position on grid
                    int livecellcount = cellchecker(i,j);

                    // if already live logic
                    if (livegrid[i,j] == 1)
                    {
                        if (livecellcount <= 1)
                        {
                            livegrid[i, j] = 0;
                        }
                        else if (livecellcount == 2 || livecellcount == 3)
                        {
                            livegrid[i, j] = 1;
                        }
                        else
                        {
                            livegrid[i, j] = 0;
                        }
                    }
                    // if dead logic
                    else
                    {
                        if (livecellcount == 3)
                        {
                            livegrid[i, j] = 1;
                        }
                        else
                        {
                            livegrid[i, j] = 0;
                        }
                    }
                }
            }
            // Calling method to display changes and repeat cycle.
            oldgrid = (int[,])livegrid.Clone();
            displaygrid(livegrid);
            Console.WriteLine("If you wish to continue enter y: ");
            string checkkey = Console.ReadLine();
            checkkey = checkkey.ToLower();
            if (checkkey == "y")
            {
                activegird(oldgrid);
            }

            else
            {
                return;
            }

        }



        // Method to check surrounding cells of a position on grid
        public int cellchecker(int x, int y)
        {

            // int var to hold number live cells 
            int livecells = 0;

            // checks +1 and +1
            if (x < 9 && y < 9)
            {
                if (livegrid[x + 1, y + 1] == 1)
                {
                    livecells++;
                }
            }

            // checks +1 and 0
            if (x < 9)
            {
                if (livegrid[x + 1, y] == 1)
                {
                    livecells++;
                }
            }
            // checks 0 and +1
            if (y<9)
            {
                if (livegrid[x, y + 1] == 1)
                {
                    livecells++;
                }

            }
            // checks -1 and -1
            if (x > 0 && y > 0)
            {
                if (livegrid[x - 1, y - 1] == 1)
                {
                    livecells++;
                }
            }
            // checks 0 and -1
            if (y>0)
            {
                if (livegrid[x, y - 1] == 1)
                {
                    livecells++;
                }
            }
            // checks -1 and 0
            if (x>0)
            {
                if (livegrid[x - 1, y] == 1)
                {
                    livecells++;
                }
            }
            // checks 1 and -1
            if (x < 9 && y > 0)
            {
                if (livegrid[x + 1, y - 1] == 1)
               {
                    livecells++;
               }
            }
            // checks -1 and 1
            if (x > 0 && y < 9)
            {
                if (livegrid[x - 1, y + 1] == 1)
                {
                    livecells++;
                }
            }
            // returns total number of live cells
            return livecells;

        }

        public void displaygrid(int[,] livegrid)
        {
            int gridcol = livegrid.GetLength(0);
            int gridrow = livegrid.GetLength(1);

            for (int i = 0; i < gridcol; i++)
            {
                for (int j = 0; j < gridrow; j++)
                {
                    Console.Write("{0,2}", livegrid[i, j]);
                }
                Console.WriteLine();
            }
        }
    }
} 
\$\endgroup\$
  • \$\begingroup\$ activegrid is recursive, doesnt seem like this is needed or intended \$\endgroup\$ – pm100 Sep 7 '16 at 21:32
  • \$\begingroup\$ Coincidentially this was recently the topic of a challenge on codefights.com. Since it's already over, you can take a look at various solutions in multiple languages (C# included) \$\endgroup\$ – Manfred Radlwimmer Sep 8 '16 at 6:22
11
\$\begingroup\$

Cell

The first thing I would do is actually not switch to a bool grid but actually make a class that represents a single cell. I'm going to call it Cell because I'm not that creative.

The cell would handle the old and new flags to start with but it would also let you easily add logic for age (some GOL implementations color the cells based on their age).

Cell would not be aware of anything else.

This will let you encapsulate the logic of the Cell in a single place.

Grid

Next, I would have a class that handles just the grid (look up Single Responsibility Principle for more reasons why). At the base level, it can easily take just a width and height variable and creates an array (of your choice) of Cell objects.

You could have Cell GetCell(x, y) that retrieves a cell at a given location. I'd also write a function List<Cell> GetAdjacentCells(x, y) to retrieve the list of cells next to the given coordinates. For GOL, that's all you really need.

Since Grid know its height and width, you don't have to get the GetLength() unless you want to.

GridUpdater

If I was writing it, I would have a separate class for updating the state. You could do this within Grid itself, but it would easier to maintain and test if you did it separately (again, see SRP).

Breaking apart the Console stuff out of there also keeps your code relatively clean (and SRP).

With GOL, you basically have two sweeps. One is to calculate the new values. The second is to move the new value into the current one.

for (int x = 0; x < Width; x++) {
  for (int y = 0; y < Height; y++) {
    var cell = grid.GetCell(x, y);
    var adjacent = grid.GetAdjacentCells(x, y);
    cell.Calculate(adjacent);
  }
}

for (int x = 0; x < Width; x++) {
  for (int y = 0; y < Height; y++) {
    var cell = grid.GetCell(x, y);
    cell.Update(); // Should be a function for encapsulation.
  }
}

The reason I suggested the logic for cell is because the Calculate function there is pretty simple.

public void Calculate(List<Cell> adjacent)
{
  // All the rules are based on the number of adjacent cells.
  var count = adjacent.Count;

  // Less than two or greater than three is always dead.
  if (count < 2 || count > 3) {
    NewState = false;
  } else {
    // For live (OldState = true) cells, they are alive. For dead ones,
    // they live only if there is exactly three. This uses the OR logic to
    // to combine the two statements together.
    NewState = OldState || count == 3;
  }
}

You don't need much more than this. Your Console stuff should be in a fourth class because it has nothing to do with the state of a cell, the size of the grid, or how the grid is updated.

Formatting

Typically C# uses PascalCase for all method and property names.

while (endgame == false) is while (!endgame).

Comprehension wise, it is great if your method names used a verb in the name. So, instead of activecell, you use CalculateNewState or Update. That way, someone new can understand what that function is supposed to do. "Active" is really just an adjective on "Cell" which is a noun.

EDIT: NewState = false; to say dead.

\$\endgroup\$
  • 5
    \$\begingroup\$ A Cell class seems like overengineering to me. \$\endgroup\$ – CrabMan Sep 7 '16 at 21:16
  • 2
    \$\begingroup\$ It can be. My reasoning for suggesting a full-blown class was because it was implied there was "more" to this than just a simple automata. Since they didn't say what was extra, I went for flexibility over performance. Plus, as a beginner, I think it is better to understand OOP concepts first and then work on optimizing based on usage. If they said they were researching patterns (which require millions of iterations), I would have given a different structure. The Cell class was more flexible and extendable. \$\endgroup\$ – dmoonfire Sep 8 '16 at 1:42
  • \$\begingroup\$ The GridUpdater seems a bit extreme to me. There's not much logic, and it'd fit easily in the Grid class. I agree that "rendering" should be split from the real logic, though. \$\endgroup\$ – Nic Hartley Sep 8 '16 at 3:59
  • 1
    \$\begingroup\$ If I knew the end goal, I would agree. After so many years though, when I heard there is "more" I have a tendency to lean toward more flexible/replaceable designs over monolithic ones. I found that it makes unit testing easier, less effort to comprehension, and it follows SOLID which is a good framework when starting code. It does make a lot of tiny classes though, but I'd rather have lots of tiny classes where a few explode in size than a 15k LOC file that grew into unmanageability. \$\endgroup\$ – dmoonfire Sep 8 '16 at 4:15
  • 1
    \$\begingroup\$ Cell is not over-engineering. Good OO design insists there be a Cell class. It is a conceptual element of the game structure and encapsulates Cell functionality such as Cell.IsAlive(). Client code must not be manipulate the sub-atomic particles of the game grid structure. If you think Cell would be too simple, well, simplicity is a natural and happy consequence of making appropriate classes; of expressing the "moving parts" of 'The Game of Life' through classes. \$\endgroup\$ – radarbob Sep 8 '16 at 14:51
8
\$\begingroup\$

This is very important: Your algorithm is wrong. Do not ask for help with style or efficiency or anything else like that until you get the algorithm right! Hint: you should not be updating the grid at the same time as you are reading from it.

Get the code compiling, then get it right, and then get it elegant and then get it fast. Always that order.

So let's start by getting the code right by identifying where it is wrong. It's wrong here:

           int livecellcount = cellchecker(i,j);
           if (livegrid[i,j] == 1)
           {
             if (livecellcount <= 1)
               livegrid[i, j] = 0;

Suppose cell i, j is 1 and the count of live neighbours is 1; specifically, the next cell over is the only live cell. Now suppose that next cell over has 2 live neighbours. What must happen when you compute the state of the next cell? It is live and it has two live neighbours, so it must stay alive. What happens in your program? You set the current cell to dead, and then you count the number of live neighbors of the next cell, which will then be 1, not 2, and the cell will die!

You need to be doing the reading on the previous grid, and the writing on the next grid, in order for this to be correct.

There is a mechanism for making sure you never make this terrible mistake again, which I shall now describe.

To address your specific questions:

A pointer on how to improve the change from one grid to another, i.e. to improve the flow of the game.

What I would like to see is first, a class that represents the grid, rather than an array. And second, that class should be immutable. A grid is not something you change. The next step in the game is a new grid. There should be a method somewhere with a signature like:

static Grid Next(Grid g)

or an instance method on the Grid class that returns a new Grid. Never change an existing grid. If you never change an existing grid and only construct new grids, then you can never make the mistake of mutating the wrong grid.

Also, you don't need to clone the existing grid. The existing grid never changes, and every cell of the new grid will be set to the correct value, so why would you need to clone the grid? Read the old grid, write the new grid, always.

Thoughts on how to improve the logic of my grid, by avoiding overuse of conditional statements.

Let's take a look at some of your logic:

if (livecellcount <= 1)
  livegrid[i, j] = 0;
else if (livecellcount == 2 || livecellcount == 3)
  livegrid[i, j] = 1;
else
  livegrid[i, j] = 0;

We immediately see that the first clause is completely unnecessary; this has identical results:

if (livecellcount == 2 || livecellcount == 3)
  livegrid[i, j] = 1;
else
  livegrid[i, j] = 0;

So now let's take a look at that in context. I'll fix the bug while I'm at it:

if (oldGrid[i,j] == 1) // Always read the ORIGINAL grid.
{
  if (livecellcount == 2 || livecellcount == 3)
    newGrid[i, j] = 1;
  else
    newGrid[i, j] = 0;
}
else
{
  if (livecellcount == 3)
    newGrid[i, j] = 1;
  else
    newGrid[i, j] = 0;
}

Can we simplify this further? Yes. We begin by noticing that the "3" condition is repeated on both branches of the outer "if", and so can be brought outside:

if (livecellcount == 3)
  newGrid[i, j] = 1;
else if (oldGrid[i,j] == 1)
{
  if (livecellcount == 2)
    newGrid[i, j] = 1;
  else
    newGrid[i, j] = 0;
}
else
  newGrid[i, j] = 0;

Can we simplify this further? There are now only two places where the new cell is live, so let's combine those two conditions:

if (livecellcount == 3 || oldGrid[i, j] == 1 && livecellcount == 2)
  newGrid[i, j] = 1;
else
  newGrid[i, j] = 0;

This is already orders of magnitude shorter. Can we make it better still? Sure! We notice that we have four numbers: 0, 1, 2 and 3. But only 2 and 3 are numbers. The 0 and 1 are alive and dead. So let's say so:

const int Alive = 1;
const int Dead = 0;
...
if (livecellcount == 3 || oldGrid[i, j] == Alive && livecellcount == 2)
  newGrid[i, j] = Alive;
else
  newGrid[i, j] = Dead;

So much easier to read. Now the numbers are actually numbers and the special values are named with their meanings.

Can we make it better? Yes. Abstract policies into methods:

static int Policy(int cell, int liveNeighbors)
{
  if (liveNeighbors == 3 || cell == Alive && liveNeighbors == 2)
    return Alive;
  return Dead;
}
...
newGrid[i, j] = Policy(oldGrid[i, j], liveNeighbors);

OMG look at how much nicer that code is compared to what you wrote. You can just read the code practically like English: the policy is that if the number of live neighbours is three, or the cell is alive and the number of live neighbours is two, then the new cell is alive. Otherwise it is dead.

General feedback to code both structurally and aesthetically, so I know what to improve in future projects.

  • Right is better than elegant. Get the code right first.
  • Strive to make your code read more like the ideas it is trying to represent. Don't use 0 and 1; use Alive and Dead.
  • Got a special rule? Abstract it away into a method that implements that rule.
  • Got some data with interesting properties? Abstract it into a class. Remember, coding is about making good abstractions.
  • Fix the spelling mistakes. "gird" makes my eyes hurt.
  • Use standard naming conventions.
  • Embrace immutable style; as we've just seen, most bugs happen when something changes in the wrong order.
  • Test things! You would have probably found your bug if you had written a test case that took a known grid, ran it forward a generation, and then compared that to the result that you had worked out by hand.
\$\endgroup\$
  • \$\begingroup\$ good answer, easy to follow and understand, I feel like I learned something after reading this answer. Thank you \$\endgroup\$ – Malachi Sep 9 '16 at 15:11
  • 1
    \$\begingroup\$ @Malachi: You are welcome. You are doing great for a first project; Game of Life was one of my early coding projects as a child, and I have come back to it many times throughout my career; there are amazing ways to implement this algorithm that will blow your mind when you learn about them. It is possible to construct programs that have grids with quadrillions of cells and compute billions of board positions per second. It sounds impossible but it is not! \$\endgroup\$ – Eric Lippert Sep 9 '16 at 15:27
7
\$\begingroup\$

With code like this

// if dead logic
else
{
    if (livecellcount == 3)
    {
        livegrid[i, j] = 1;
    }
    else
    {
        livegrid[i, j] = 0;
    }
}

You could make it a single one line inside of the else block by using what is called a ternary statement.

A ternary statement is structured like this

variableName = Conditional ? if true do this : if false do this;

so your code would look like this

livegrid[i,j] = livecellcount == 3 ? 1 : 0;

Another thing that you should keep in mind is C#'s naming standards

as you can see the classes are PascalCase Program but the class that you created is not PascalCase.

personally I prefer SHOUTCASE for Constants, but I think it is just a matter of preference for the developer/team

What ever style you (or your team) go with, make sure to stay consistent.

\$\endgroup\$
  • \$\begingroup\$ I disagree with constants being uppercase. Microsoft states all public fields and properties should be PascalCase. Since most constants are public and static (though read only), I have always read that as being not uppercase. Likewise, this StackOverflow question points out that StyleCop also uses PascalCase. \$\endgroup\$ – dmoonfire Sep 7 '16 at 19:21
  • \$\begingroup\$ @dmoonfire the answer that points to StyleCop is is near 9 years old. I have heard bad things about StyleCop and don't know any developers that use it. Constants are usually there in the place of a Magic Number and have a specific purpose, they should not be changed and should not be accessible outside of the class \$\endgroup\$ – Malachi Sep 7 '16 at 19:26
  • \$\begingroup\$ Microsoft's Framework Design Guide also says it, but that requires buying it but it is referenced in this post from 2015. "The PascalCasing convention, used for all identifiers except parameter names, capitalizes the first character of each word (including acronyms over two letters in length)" Enums are are also effectively constants and PascalCase. I have yet to find a Microsoft standard that uses uppercase that isn't ported from another language or Win32. \$\endgroup\$ – dmoonfire Sep 7 '16 at 19:32
  • \$\begingroup\$ Enums are not effectively constants, wikipedia calls them a data type consisting of a set of named values called elements, members, enumeral, or enumerators of the type. Microsoft --> an enum is a distinct type that consists of Constants. \$\endgroup\$ – Malachi Sep 7 '16 at 19:37
  • 1
    \$\begingroup\$ the point is - use some form of naming convention that is not simply all lowercase with no punctuation, and be consistent. Look at microsoft runtime source for a large body of c# code for at least one coding style referencesource.microsoft.com/#System/compmod/system/… \$\endgroup\$ – pm100 Sep 7 '16 at 21:29
5
\$\begingroup\$

Another thing: you could also make this code a lot easier to read:

        if (x < 9 && y < 9)
            if (livegrid[x + 1, y + 1] == 1)
                livecells++;
        if (x < 9)
            if (livegrid[x + 1, y] == 1)
                livecells++;
            if (livegrid[x, y + 1] == 1)
                livecells++;
        if (x > 0 && y > 0)
            if (livegrid[x - 1, y - 1] == 1)
                livecells++;
        if (y>0)
            if (livegrid[x, y - 1] == 1)
                livecells++;
        if (x>0)
            if (livegrid[x - 1, y] == 1)
                livecells++;
        if (x < 9 && y > 0)
            if (livegrid[x + 1, y - 1] == 1)
                livecells++;
        if (x > 0 && y < 9)
            if (livegrid[x - 1, y + 1] == 1)
                livecells++;
        return livecells;

How? Break it up!

const XSize = 9;
const YSize = 9;
bool ValidX(int x) => 0 <= x && x < XSize;
bool ValidY(int y) => 0 <= y && y < YSize;

Note that this uses a new syntax for C# 6; in C# 5 you'd say

bool ValidX(int x)
{
  return 0 <= x && x < XSize;
}

The new syntax is just shorter for small functions.

Now we can say:

int ValidOrDead(int x, int y) => ValidX(x) && ValidY(y) ? grid[x, y] : Dead;
int North(int x, int y) => ValidOrDead(x, y-1);
int South(int x, int y) => ValidOrDead(x, y+1);
int West(int x, y) => ValidOrDead(x-1, y);

and so on. So now we can say:

int LiveNeighbors(int x, int y) => 
  North(x, y) + South(x, y) + West(x, y) + ...

Again look at how much easier it is to read code when you break it up into tiny methods, each of which does one thing correctly.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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