Ruby Sudoku solver

This week's weekend challenge #3 seemed like a great opportunity to learn Ruby! Unfortunately, my workload and looming vacation did not cooperate. :( The puzzle will make forced moves automatically, but I was hoping to implement rules such as Naked Pairs before resorting to brute force.

But there's plenty of code to make a review worthwhile, and I'm hoping some experienced Ruby coders will give it a shot. I have included a few test puzzles (easy and medium solve to completion), and it's easy to add more. See the bottom for a sample test run.

The real meat of the algorithm is the interaction between the Cell and Group instances. Board merely instantiates everything and applies the initial puzzle state.

I'm looking for feedback on Ruby style (e.g. "avoid and and or as they are confusing"), correct use of idioms and the standard library, code organization, etc. This is a Ruby learning exercise, so I want to avoid picking up bad habits early. Since I haven't implemented any brute-force or complex solving yet, performance isn't an issue.

Board.rb

```
require 'Cell'
require 'Group'
require 'Inspector'

module Sudoku

# Builds the cells and groups and provides an API for setting up and interacting with the puzzle.
#
# Several debugging methods are provided that are being extracted to the inspector.
class Board

# Builds the board structure and initializes an empty puzzle.
#
# @param start [String] optional initial values passed to #setup
def initialize(start = nil)
@cells = Array.new(9) { |r| Array.new(9) { |c| Cell.new r, c } }
@rows = Array.new(9) { |i| Group.new :row, i }
@cols = Array.new(9) { |i| Group.new :col, i }
@boxes = Array.new(9) { |i| Group.new :box, i }
(0..8).each do |i|
connect @rows[i], row_cells(i)
connect @cols[i], col_cells(i)
connect @boxes[i], box_cells(i)
end
setup start if start
end

# Sets the initial values of the puzzle.
#
# P

It's definitely a nice start. But it's funny; I can tell you're coming to Ruby from Java :)

I don't mean anything negative by that, by the way. It's just interesting how some subtle clues here and there give it away.

Anyway, if you want to develop good habits early, unit testing and TDD/BDD would be my first recommendations. Personally, I neglected it for too long, and it's hard to make a habit of it later.

I'm sure you know all the reasons to test, but it's doubly great if you're still learning and want to try different solutions out. It's also just nice to learn it in a situation where you already have a spec (as opposed to developing something brand new where you're figuring it out feature spec by feature spec, then figuring out what tests to apply, then the code, etc.).

I'm a fan of rspec myself, but there are plenty of testing solutions out there, like test/unit which is included in Ruby's standard library.

But let's move on to the code itself, starting with the Board class:

I'd suggest more (private or otherwise) accessor methods rather than accessing instance variables directly. To begin with, you can just do attr_reader :cells (for instance), and leave it at that. But accessor methods make everything easier later on. This is probably not something you'll have to maintain over a long time, but you asked for good habits, and the idea is that you couple your code to a method, which is more flexible than coupling it directly to a variable.

Speaking of, I might move the cells/rows/cols/boxes initialization out of the initialize method, favoring reader methods (getters) that use to the ||= operator to initialize them, if they haven't been defined. E.g.

def rows
   @rows ||= Array.new(9) { |i| Group.new :row, i }
 end

That way, you're guaranteed to always have rows available, and the code to create them has been moved, slimming down the initializer method (try to be zealous about keeping method line counts low; like aim for, like, 5 lines max. Won't always work, but try it as an exercise).

I might actually go further to DRY (Don't Repeat Yourself) the code, by making a helper method just to return a new array of groups

def rows
   @rows ||= create_groups(:row)
 end

 # ...

 def create_groups(type)
   Array.new(9) { |i| Group.new type, i }
 end

I know, it seems sort of pointless, but what if, for instance, you want different board sizes later? You could add a size argument to create_types, or (better) have it access an instance variable (again, directly or through a reader method) to get the size. Of course the actual solving would need to take the different size into account, but the basics of building the board would "just work".

Another general note would be to use longer variable names. It doesn't cost you anything to replace r, c and n with row, column and number (or better yet value for the last one, as you can conceivably use any symbol set for a sudoku puzzle). It just makes everything way more explicit.

Basically, in most Ruby code, you'll typically only see a few @s (confined to accessor methods) and virtually no single-letter variables.

Which leads me to Board#setup. Again there are some one-letter variables going on here, but moreover you're doing some un-Rubyesque looping. You should go read though the docs for Array and the Enumerable module, as that's one of the most handy parts of Ruby, in my opinion. I know a lot of my own early Ruby code could have benefitted a lot if I'd had the patience to just go through those APIs.

Here's one, more Ruby-like, way to achieve the same thing:

def setup(puzzle)
  puzzle.split.each_with_index do |line, row|
    line.scan(/./).each_with_index do |char, column|
      set(row, column, char.to_i, :start) if char =~ /\d/
    end
  end
end

Basically, you practically never need to track and manually increment indices when looping, and only very rarely does a block need to have side-effects (like pushing items into an array); Ruby can get very functional.

For instance, you could consider map'ing the lines and chars directly to the @cells, as it follows the same 2D-structure as the puzzle text.

Your Board#set method is also a bit un-Rubyesque. For one, it's got a lot of arguments, which couples it rather tightly to the rest of the code. There's also the symbol you pass according to the state of the puzzle (a state you must explicitly supply). I'd recommend separate methods for setting/force setting the cells. That's a plain refactoring concern, and as I said I won't delve too deep into that, but here are some ideas:

• changing the order of things to a cell isn't initialized until you've got a value to feed its constructor

• set and set! and choose methods for the various ways of setting the value; more explicit and expressive that way.

Speaking of method names, you could consider using [] in your cell-specific methods, to make Board appear more like a 2D array. E.g.

``

StackExchange Code Review Q#37723, answer score: 6