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

Cleaning up code for Conway's Game of Life

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

Problem

I've implemented a Ruby version of Conway's Game of Life that functions correctly and passes all my tests. I was hoping to get some advice on how I could clean up the code for readability and efficiency.

For anyone who doesn't know the rule to the Game of life, they are as follows:



  • Any live cell with fewer than two live neighbours dies, as if by needs caused by underpopulation.



  • Any live cell with more than three live neighbours dies, as if by overcrowding.



  • Any live cell with two or three live neighbours lives, unchanged, to the next generation.



  • Any dead cell with exactly three live neighbours becomes a live cell.




(In my implementation, 1 is an alive cell and 0 is a dead cell).

```
class Game_of_life
def initialize(hash)
@test = nil
str = hash[:string]
size = hash[:size]
@length = nil
@board = []
if str != nil
str.each_char {|c| @board 3)
@board[index] = 1 if organism == 0 && neighbours[:alive] == 3
end
if @test
return @board.join("")
else
sleep 0.5
puts "\e[H\e[2J"
@board.each_slice(@length) {|row| p row }
check_board
end
end

def choose_neighbour_set(index)
neighbour_set = [-(@length), -(@length - 1), 1, (@length+1), @length, (@length-1), -1, -(@length + 1)]
neighbour_set.delete_if{|position| [-(@length + 1), -(@length),-(@length - 1)].include?(position)} if index ((@length*(@length-1)) -1)
neighbour_set.delete_if{|position| [-(@length + 1),-1,(@length-1)].include?(position)} if index % @length == 0 || index == 0
neighbour_set.delete_if{|position| [(@length+1),1,-(@length - 1)].include?(position)} if (index + 1) % @length == 0 && index != 0
return neighbour_set
end
end

# Uncomment to watch the game of life unfold!
# Game_of_life.new({size: 25}).check_board

# >>>>>>>>>>>>>>>>>>>>> Driver code

# Test string one
arr = "100100111110100011110110001100110010000010111101100010010111110110111100110000011"
game = Game_of_life.new

Solution


  • Ruby convention is to use CamelCase for Classnames (GameOfLife)



  • Since "size" and "str" are mutably exclusive parameters, using a hash parameter here makes the API confusing



  • Using flags like "@test" to define object behavior is bad style (antipattern)



  • Ruby doesn't have tail recursion, so you shouldn't do recursive calls without end conditions



To make this more OOP like and self documenting you can define a base class for the basic initialization:

class GameOfLife
  def initialize(length)
    @length = length # Why not call this "size"?
    @board = []
  end

  def check_board
    # Implemented as above without the "if" statement
  end

  def choose_neighbour_set(index)
    # As above
  end
end


Then you can use subclasses for specific implementations

class RandomGameOfLife < GameOfLife
  def initialize(size)
    super(size)
    (size*size).times do
      @board << [0,1].sample
    end
  end

  def iterate_and_output_board
    while true # Iterate till ^C?
      check_board
      sleep 0.5
      puts "\e[H\e[2J"
      @board.each_slice(@length) {|row| p row }
    end
  end
end

class PredefinedGameOfLife < GameOfLife # Or use a better name here
  def initialize(str)
    super(Math.sqrt(str.length))
    str.each_char {|c| @board << c.to_i}
  end

  def check_board
    # This is just an example how to overwrite method behavior
    # Defining another method using "check_board" might be better here
    super
    @board.join("")
  end
end


Somebody else might cover the efficiency aspect. I don't want to intermix the aspects here and its better to focus on readability first.

Code Snippets

class GameOfLife
  def initialize(length)
    @length = length # Why not call this "size"?
    @board = []
  end

  def check_board
    # Implemented as above without the "if" statement
  end

  def choose_neighbour_set(index)
    # As above
  end
end
class RandomGameOfLife < GameOfLife
  def initialize(size)
    super(size)
    (size*size).times do
      @board << [0,1].sample
    end
  end

  def iterate_and_output_board
    while true # Iterate till ^C?
      check_board
      sleep 0.5
      puts "\e[H\e[2J"
      @board.each_slice(@length) {|row| p row }
    end
  end
end

class PredefinedGameOfLife < GameOfLife # Or use a better name here
  def initialize(str)
    super(Math.sqrt(str.length))
    str.each_char {|c| @board << c.to_i}
  end

  def check_board
    # This is just an example how to overwrite method behavior
    # Defining another method using "check_board" might be better here
    super
    @board.join("")
  end
end

Context

StackExchange Code Review Q#30381, answer score: 3

Revisions (0)

No revisions yet.