Skip to main content
[Edit removed during grace period]
Source Link
Maxim
  • 2.5k
  • 9
  • 17
added 674 characters in body
Source Link
Maxim
  • 2.5k
  • 9
  • 17

The first bad thing you do is a lot of string concatenations like

_outputGrid += "1";

which cause many memory reallocations. Since during one game grid size is a constant number you can save it at the constructor, and then just create array for result and set its elements by index.

_outputGrid = new char[_gridSize * _gridSize];
...
_outputGrid[currentIndex] = '0';

At the end of the CalculateNextGrid method convert this array to string and return it:

return new string(_outputGrid);

I suggest to use array of bool's as internal storage for result instead of a string. In my opinion true and false represent the state of a cell (alive or dead) more clearly. You can even use array of enum values like:

public enum CellState
{
    Alive,
    Dead
}

_outputGrid[currentIndex] = CellState.Alive;

You can go further and create method like:

private void ReviveCell(int index) => _outputGrid[index] = CellState.Alive;

It would be better to extract magic conditions to some methods like:

private static bool ShouldBeDeadByUnderpopulation(int count) => count < 2;

private static bool ShouldBeDeadByOverpopulation(int count) => count > 3;

and so on.

And instead of condition like

if (inputGrid[currentIndex + _up    + _right]   == '1')

use methods too

if (IsAlive(value))

With all these changes your code will look like a story where all is clear :)

if (IsAlive(currentCell))
{
    if (ShouldBeDeadByUnderpopulation(_cellCount ) ||
        ShouldBeDeadByOverpopulation(_cellCount))
    {
        KillCell(currentIndex);
    }
    else
    {
        // Note that this code is redundant since cell is alive at the moment
        ReviveCell(currentIndex);
    }
}

The first bad thing you do is a lot of string concatenations like

_outputGrid += "1";

which cause many memory reallocations. Since during one game grid size is a constant number you can save it at the constructor, and then just create array for result and set its elements by index.

_outputGrid = new char[_gridSize * _gridSize];
...
_outputGrid[currentIndex] = '0';

At the end of the CalculateNextGrid method convert this array to string and return it:

return new string(_outputGrid);

I suggest to use array of bool's as internal storage for result instead of a string. In my opinion true and false represent the state of a cell (alive or dead) more clearly.


It would be better to extract magic conditions to some methods like:

private static bool ShouldBeDeadByUnderpopulation(int count) => count < 2;

private static bool ShouldBeDeadByOverpopulation(int count) => count > 3;

and so on.

And instead of condition like

if (inputGrid[currentIndex + _up    + _right]   == '1')

use methods too

if (IsAlive(value))

The first bad thing you do is a lot of string concatenations like

_outputGrid += "1";

which cause many memory reallocations. Since during one game grid size is a constant number you can save it at the constructor, and then just create array for result and set its elements by index.

_outputGrid = new char[_gridSize * _gridSize];
...
_outputGrid[currentIndex] = '0';

At the end of the CalculateNextGrid method convert this array to string and return it:

return new string(_outputGrid);

I suggest to use array of bool's as internal storage for result instead of a string. In my opinion true and false represent the state of a cell (alive or dead) more clearly. You can even use array of enum values like:

public enum CellState
{
    Alive,
    Dead
}

_outputGrid[currentIndex] = CellState.Alive;

You can go further and create method like:

private void ReviveCell(int index) => _outputGrid[index] = CellState.Alive;

It would be better to extract magic conditions to some methods like:

private static bool ShouldBeDeadByUnderpopulation(int count) => count < 2;

private static bool ShouldBeDeadByOverpopulation(int count) => count > 3;

and so on.

And instead of condition like

if (inputGrid[currentIndex + _up    + _right]   == '1')

use methods too

if (IsAlive(value))

With all these changes your code will look like a story where all is clear :)

if (IsAlive(currentCell))
{
    if (ShouldBeDeadByUnderpopulation(_cellCount ) ||
        ShouldBeDeadByOverpopulation(_cellCount))
    {
        KillCell(currentIndex);
    }
    else
    {
        // Note that this code is redundant since cell is alive at the moment
        ReviveCell(currentIndex);
    }
}
Source Link
Maxim
  • 2.5k
  • 9
  • 17

The first bad thing you do is a lot of string concatenations like

_outputGrid += "1";

which cause many memory reallocations. Since during one game grid size is a constant number you can save it at the constructor, and then just create array for result and set its elements by index.

_outputGrid = new char[_gridSize * _gridSize];
...
_outputGrid[currentIndex] = '0';

At the end of the CalculateNextGrid method convert this array to string and return it:

return new string(_outputGrid);

I suggest to use array of bool's as internal storage for result instead of a string. In my opinion true and false represent the state of a cell (alive or dead) more clearly.


It would be better to extract magic conditions to some methods like:

private static bool ShouldBeDeadByUnderpopulation(int count) => count < 2;

private static bool ShouldBeDeadByOverpopulation(int count) => count > 3;

and so on.

And instead of condition like

if (inputGrid[currentIndex + _up    + _right]   == '1')

use methods too

if (IsAlive(value))