patternrubyMinor
Cleaning up code for Conway's Game of Life
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:
(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
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
endThen 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
endSomebody 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
endclass 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
endContext
StackExchange Code Review Q#30381, answer score: 3
Revisions (0)
No revisions yet.