Ruby: Refactoring a complicated nested-loop method

Question

I'm trying to get rid of duplication in my code. I have a method that populates a checkerboard with checkers:

def populate_checkers
  evens = [0, 2, 4, 6]
  odds  = [1, 3, 5, 7]

  0.upto(2) do |x_coord|
    if x_coord.even?
      evens.each do |y_coord|
        red_checker = Checker.new(x_coord, y_coord, :red)
        @board[x_coord][y_coord] = red_checker 
      end
    elsif x_coord.odd?
      odds.each do |y_coord|
        red_checker = Checker.new(x_coord, y_coord, :red)
        @board[x_coord][y_coord] = red_checker 
      end
    end
  end

  5.upto(7) do |x_coord|
    if x_coord.even?
      evens.each do |y_coord|
        black_checker = Checker.new(x_coord, y_coord, :black)
        @board[x_coord][y_coord] = black_checker 
      end
    elsif x_coord.odd?
      odds.each do |y_coord|
        black_checker = Checker.new(x_coord, y_coord, :black)
        @board[x_coord][y_coord] = black_checker 
      end
    end
  end
end

How can I remove duplication and still get the precise behavior I need?

side note: from a functional-programming perspective this kind of code is very, very bad. You call a method and "magically" some instance variable (@board) is filled, alas, referential transparency going down the hill. Better write methods that get arguments and return something: `@board = build_board`. More on FP with Ruby: http://www.slideshare.net/tokland/functional-programming-with-ruby-9975242 — tokland, Dec 07 '11 at 13:54

score 3 · Accepted Answer · answered Dec 07 '11 at 12:48

You can try to extract a method and then extract a block into lambda. Then your code will be readable and loose of duplication

def populate_checkers
  0.upto(2) do |x_coord|
    populate_checker(x_coord, :red)
  end

  5.upto(7) do |x_coord|
    populate_checker(x_cord, :black)
  end
end

def populate_checker(x_coord, color)
  evens = [0, 2, 4, 6]
  odds  = [1, 3, 5, 7]

  apply_checker = lambda do |y_coord|
   checker = Checker.new(x_coord, y_coord, color)
   @board[x_coord][y_coord] = checker
  end

  if x_coord.even?
    evens.each(&apply_checker)
  elsif x_coord.odd?
    odds.each(&apply_checker)
  end
end

andrew12 · Answer 2 · 2011-12-07T13:08:52.573

def populate_checkers
  evens = [0, 2, 4, 6]
  odds  = [1, 3, 5, 7]

  [0.upto(2), 5.upto(7)].each_with_index do |enum, i|
    enum.each do |x_coord|
      (x_coord.even? ? evens : odds).each do |y_coord|
        checker = Checker.new(x_coord, y_coord, i == 0 ? :red : :black)
        @board[x_coord][y_coord] = checker 
      end
    end
  end
end

There's probably a better way to do the counting bit, but that's what I got.

Here's a possibly better solution...

def populate_checkers
  { :red => (0..2), :black => (5..7) }.each do |color, range|
    range.each do |x_coord|
      (x_coord.even? ? 0 : 1).step(7, 2) do |y_coord|
        checker = Checker.new(x_coord, y_coord, color)
        @board[x_coord][y_coord] = checker 
      end
    end
  end
end

score 1 · Answer 3 · answered Dec 07 '11 at 13:02

1

0.upto(2) do |x|
  0.upto(7) do |y|
    @board[x][y]=Checker.new(x, y, :red) if (x+y).even? 
  end
end

This is only for the reds.

answered Dec 07 '11 at 13:02

steenslag

79,051
16
138
171

Ruby: Refactoring a complicated nested-loop method

3 Answers3