5
\$\begingroup\$

I'm just starting with Ruby and decided to do two different implementations of Conway's Game of Life. In this first one I'm not using cells as a separate class and instead just track everything as a location on the board, using a state pattern I guess.

Anyway, I want this code to display good Object-Oriented programming basics as well as construction via test-driven development, using rspec.

Please critique away and tell me if it's under/over engineered for the problem, not extensible enough or too extended, as well as if my TDD progression was worthwhile, did I do enough or too few tests, or maybe not the right tests.

board.rb:

class Board
attr_accessor :width,:height
 def initialize(cells=[], width = 3, height = 3)
 @cells = cells
 @height = height
 @width = width
 end
 def live_cells
 @cells
 end
 def is_alive?(x,y)
 @cells.include?([x,y])
 end
 def neighbors_to(x,y)
 @cells.select do |cell_x, cell_y|
 dx = (x-cell_x).abs
 dy = (y-cell_y).abs
 #If the tuple x,y is a neighbor of that cell then the following is true
 dx <= 1 and dy <= 1 and not (dx == 0 and dy == 0)
 end.length
 #We only care how many there are and do not care which cells in particular
 end
 def evolve
 survivors = []
 #Scan the entire board (may be able to be more clever here but you need to check each dead cell for ressurection)
 @width.times do |x|
 @height.times do |y|
 survivors << [x,y] if should_survive?(x,y) or should_ressurect?(x,y)
 end
 end
 #Populating the new board from survivors allows reference to the current generation while populating the next generation
 Board.new(survivors, self.width,self.height)
 end
 def should_survive?(x,y)
 (neighbors_to(x,y) == 2 or neighbors_to(x,y) == 3) and is_alive?(x,y)
 end
 def should_ressurect?(x,y)
 neighbors_to(x,y) == 3 and not is_alive?(x,y)
 end
 def print_board
 puts "Current Generation"
 puts
 @width.times do |x|
 @height.times do |y|
 print "#{is_alive?(x,y) ? '#' : '-'}"
 end
 puts
 end
 puts
 end
end

board_spec.rb:

require 'spec_helper'
describe Board do
 context "#new" do
 it "should be created empty if passed empty array" do
 Board.new([]).live_cells.should be_empty
 end
 it "should be created empty if passed nothing at all" do
 Board.new
 end
 it "should contain the cells created with it" do
 Board.new([[1,1],[2,2]]).live_cells.should be ==[[1,1],[2,2]]
 end
 end
 context "#evolve" do
 it "should evolve an empty board if empty" do
 next_gen = next_generation_for([])
 next_gen.should be_empty
 end
 it "should kill a lonely cell (< 2 neighbors)" do
 next_gen = next_generation_for([[1,1]])
 next_gen.should be_empty
 end
 it "should keep a cell with 2 neighbors alive" do
 #pending("We need to count neighbors")
 next_gen = next_generation_for([[1,1],[1,2],[1,0]])
 next_gen.should include([1,1])
 end
 it "should keep a cell with 3 neighbors alive" do
 #pending("We need to count neighbors")
 next_gen = next_generation_for([[1,1],[1,2],[1,0],[0,0]])
 next_gen.should include([1,1])
 end
 it "should revive a dead cell with 3 neighbors (reproduction)" do
 #pending("Dimensions of the board to find dead cells")
 next_gen = next_generation_for([[1,0],[0,1],[2,1]])
 next_gen.should include([1,1])
 end
 it "should kill any cell with more than 3 neighbors" do
 next_gen = next_generation_for([[0,1],[2,1],[1,2],[1,0],[1,1]])
 next_gen.should_not include([1,1])
 end
 end
 def next_generation_for(seed,width = 3, height = 3)
 board = Board.new(seed, width, height)
 board.evolve.live_cells
 end
 it "should be able to handle the blinker formation 10 times" do
 seed = [[1,2],[1,1],[1,0]]
 n = 0
 while n<=10
 next_gen = next_generation_for(seed)
 next_gen.should satisfy{|cells| cells =~[[0,1],[1,1],[2,1]] or [[1,2],[1,1],[1,0]]}
 seed = next_gen
 n+=1
 end
 end
 it "should be able to handle the block formation 10 times" do
 seed = [[1,1],[2,1],[2,2],[1,2]]
 n = 0
 while n <= 10
 next_gen = next_generation_for(seed)
 next_gen.should =~ [[1,1],[2,1],[2,2],[1,2]]
 seed = next_gen
 n+=1
 end
 end
 it "should be able to handle the beehive formation 10 times" do
 seed = [[1,0],[2,0],[3,1],[2,2],[1,2],[0,1]]
 n = 0
 while n <= 10
 next_gen = next_generation_for(seed,4,4)
 next_gen.should =~ [[1,0],[2,0],[3,1],[2,2],[1,2],[0,1]]
 seed = next_gen
 n+=1
 end
 end
 it "should be able to print the current board" do
 board = Board.new([[1,1],[0,1],[0,2]],5,5)
 board.print_board
 end
end
describe Board, "counting neighbors" do
 it "should find zero neighbors to a solitary cell" do
 board = Board.new([[1,1]])
 board.neighbors_to(1,1).should == 0
 end
 it "should find one neighbor to a single cell" do
 board = Board.new([[1,1]])
 board.neighbors_to(0,0).should == 1
 board.neighbors_to(0,1).should == 1
 board.neighbors_to(0,2).should == 1
 board.neighbors_to(1,2).should == 1
 board.neighbors_to(2,2).should == 1
 board.neighbors_to(2,1).should == 1
 board.neighbors_to(2,0).should == 1
 board.neighbors_to(1,0).should == 1
 end
 it "should find two neighbors to cells if needed" do
 board = Board.new([[0,1],[2,2]])
 board.neighbors_to(1,1).should == 2
 board.neighbors_to(1,2).should == 2
 end
 it "should find three neighbors to cells if needed" do
 board = Board.new([[0,1],[2,2],[1,2]])
 board.neighbors_to(1,1).should == 3
 board.neighbors_to(2,2).should == 1
 end
end

spec_helper just has a pointer to require board.rb. I'm trying to get into this practice for larger programs so adding new classes won't require me to edit individual spec files. I also have a file, run_game.rb, that simply allows me to choose different patterns and iterates the game.

Some things that stand out to me:

  • There is very little user fault tolerance, i.e. I can give it cells on a board that's too small and it runs just fine, it just never displays cells off the board. Is it worth the extra complication to solve such problems, i.e. make it more durable to a user or is this structure enough to communicate a foundation understanding of OO programming?
  • Is there repetition in the live_cells method? I could just use attr_accessor :cells but that's far less readable.
  • Would it be a "better" implementation from an OO programming standpoint to have cells as a class as well?
Malachi
29k11 gold badges86 silver badges188 bronze badges
asked Jan 10, 2013 at 23:52
\$\endgroup\$
1
  • \$\begingroup\$ Small detail, but important: Never x-y, always x - y. Conversly, x = -y, never x = - y. \$\endgroup\$ Commented Jan 30, 2013 at 18:10

2 Answers 2

3
\$\begingroup\$

Your code looks pretty good to me. I specially like how you don't update inplace a Board but create new ones on each iteration. And the OOP aspects of it seem also ok, I wouldn't write a Cell class when a pair [x, y] does just fine and there are not many methods you can encapsulate within (other than dead/alive, which isn't helpful as your existing cells are those alive). Some notes on details:

  • def initialize(cells=[], width = 3, height = 3). I wouldn't recommend positional optional arguments, it gets messy pretty quickly. Maybe def initialize(options = {}) with key arguments? Note that Ruby 2.0 has keyword arguments: def initialize(cells: [], width: 3, height: 3)

  • def is_alive?(x,y) Add some spaces to let it breathe, it enhances readability: def is_alive?(x, y)

  • dx <= 1 and dy <= 1 and not (dx == 0 and dy == 0). and/or are used for control flow, &&/|| for boolean expressions. And ! is more idiomatic than not.

  • @cells.select { ... }.length note that this may be written @cells.count { ... }.

  • survivors = []. Your code is almost functional, don't give up:

    survivors = (0...@width).to_a.product(0...@height).select do |x, y|
     should_survive?(x, y) || should_ressurect?(x, y)
    end
    
  • print_board. I'd write it differently (also in functional style):

    def print_board
     matrix = 0.upto(@width - 1).map do |x|
     0.upto(@height - 1).map do |y|
     is_alive?(x, y) ? '#' : '-'
     end.join
     end.join("\n")
     puts "Current Generation\n\n#{matrix}\n"
    end
    
answered Jan 11, 2013 at 19:54
\$\endgroup\$
1
  • \$\begingroup\$ Great suggestions. I'll implement these as soon as I can and update the original post. I'm working on making my blocks more functional every day. It's very helpful to try it one way then push it to functional with your suggestions or on my own. Learning by doing ftw. \$\endgroup\$ Commented Jan 15, 2013 at 0:09
5
\$\begingroup\$

I won't talk about style, because @tokland's comments are spot-on.

However, some quick notes about efficiency. For large board sizes, this implementation is inefficient.

For example, neighbors_to scans every cell on the board, and it is called multiple times. It is called twice from the should_survive? method:

def should_survive?(x,y)
 (neighbors_to(x,y) == 2 or neighbors_to(x,y) == 3) and is_alive?(x,y)
end

You can reduce the call to one, and swap the order so you don't have to call it at all if the cell is alive:

def should_survive?(x,y)
 is_alive?(x,y) and [2,3].include? neighbors_to(x,y)
end

The implementation of neighbors_to should simply collect each of the surrounding eight (maximum) spaces rather than scan the entire set of live cells.

This also means you need to be able to query a cell directly, rather than scan all live cells as your is_alive? method currently does. I would recommend representing a board using a 2D array of spaces. I'd probably rename @cells to @live_cells to avoid confusion.

answered Feb 5, 2013 at 12:37
\$\endgroup\$
1
  • \$\begingroup\$ Good suggestion. I'll change the neighbors_to method to check if eight surrounding cells are in the current set of @cells list rather than going through all of the cells. \$\endgroup\$ Commented Feb 11, 2013 at 18:11

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.