4
\$\begingroup\$

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
\$\endgroup\$
2
  • \$\begingroup\$ The condition on if last.all?... spans multiple lines and is hard to read, consider using the "then" keyword \$\endgroup\$ – Devon Parsons Jan 19 '15 at 13:34
  • \$\begingroup\$ poi.nums = poi.nums - [num] should be poi.nums -= [num]. Also poi is a bad var name, it's absolutely unclear what it is. \$\endgroup\$ – Devon Parsons Jan 19 '15 at 13:34
1
\$\begingroup\$

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
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.