Ruby Sudoku X-wing solution

Question

I have written a Sudoku solver, for the euler problem and also packaged it into a ruby gem. In my solution strategies I do a lot of iteration (double/triple nested loops, I am coming from Java/C). One method in particular has a lot of if checks and iteration. Would it be better to package this iteration into an enumerator method or another approach all together? I am trying to learn the ruby way. To be clear all the code works but I fear it is written terribly.

All the code can be seen here.

This is from grid.rb

  def x_wing
    box_line_reduction
    remaining_points.each do |point|
      point.nums.each do |num|
        [:x, :y].each do |symbol|
          arr = @points.select{ |p| p.nums.include?(num) && p.send(flip(symbol)) == point.send(flip(symbol)) && p.value == 0  }
          if arr.count == 2 && @points.select { |p| p.value == num && p.send(flip(symbol)) == point.send(flip(symbol)) }.count == 0
            last = @points.select { |p| p.nums.include?(num) &&
                                    arr.map{ |a| a.send(symbol) }.include?(p.send(symbol)) &&
                                    (!arr.include?(p)) &&
                                    p.value == 0 && check_row(p.y,p,num,symbol) }
            if last.all? { |x| x.send(flip(symbol)) == last.first.send(flip(symbol)) } &&
               last.count == 2 && 
               @points.select { |p| p.value == num && p.send(flip(symbol)) == last.first.send(flip(symbol)) }.count == 0
                 final = arr + last 
                 places = final.map { |m| m.send(symbol) }.uniq
                 remaining_points.select { |p| places.include?(p.send(symbol)) && (!final.include?(p)) }.each do |poi|
                 poi.nums = poi.nums - [num]
              end
            end
          end
        end
      end 
    end

Answer 1

Your Ruby code is so dense as to be unreadable. What you want to do is separate each component into a separate method, and use descriptive names for the methods, which are a way of commenting your code. I can't figure out what the methods and tests do, but you want to replace second_test and such with clear multi-word underscored descriptions of that test, such as any_valid_points?.

Separately, you want to use the -= and zero? methods. Rather than using an if statement without an else, you want to use a guard clause. {} blocks should only be used when they fit on one (80 character!!) line. Finally, I really recommend the use of Rubocop, which will give you all these pointers, and also push you to reduce the Assignment Branch Condition complexity of the remaining methods.

def initialize
  @arr = []
end

def x_wing
  box_line_reduction
  remaining_points.each do |point|
    point.nums.each do |num|
      [:x, :y].each { |symbol| process(symbol, num) }
    end
  end
end

def process(symbol, num)
  @arr = first_method(symbol, num)
  next unless second_test?(symbol, num)
  last = second_method(symbol, num)
  third_method(last) if third_test?(last)
end

def first_method(symbol, num)
  @points.select do |p|
    p.nums.include?(num) &&
      p.send(flip(symbol)) == point.send(flip(symbol)) &&
      p.value == 0
  end
end

def second_test?(symbol, num)
  @arr.count == 2 &&
    @points.select do |p|
      p.value == num && p.send(flip(symbol)) == point.send(flip(symbol))
    end.zero?
end

def second_method(symbol, num)
  @points.select do |p|
    p.nums.include?(num) &&
      @arr.map { |a| a.send(symbol) }
        .include?(p.send(symbol)) &&
      (!arr.include?(p)) &&
      p.value == 0 && check_row(p.y, p, num, symbol)
  end
end

def third_test?(last)
  last.all? { |x| x.send(flip(symbol)) == last.first.send(flip(symbol)) } &&
    last.count == 2 &&
    @points.select do |p|
      p.value == num && p.send(flip(symbol)) == last.first.send(flip(symbol))
    end.zero?
end

def third_method(last)
  final = @arr + last
  places = final.map { |m| m.send(symbol) }.uniq
  remaining_points
    .select { |p| places.include?(p.send(symbol)) && (!final.include?(p)) }
    .each { |poi| poi.nums -= [num] }
end