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

• Devon Parsons: The condition on if last.all?... spans multiple lines and is hard to read, consider using the "then" keyword
• Devon Parsons: poi.nums = poi.nums - [num] should be poi.nums -= [num]. Also poi is a bad var name, it's absolutely unclear what it is.

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