Ruby Sudoku solver

Question

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.

Update: 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.
    #
    # Place each row on a separate line and nine cell values on each row.
    # Whitespace is ignored around the row on each line. Each cell value
    # may be a digit [1-9] or a period (.) if empty.
    #
    # @param start [String] optional initial values passed to #setup
    #
    # @example Nine queens
    #   .....9...
    #   ...9.....
    #   ......9..
    #   9........
    #   .......9.
    #   .9.......
    #   ....9....
    #   ..9......
    #   ........9
    def setup(puzzle)
      r = 1
      puzzle.split.each do |line|
        c = 1
        line.each_char do |n|
          set(r, c, n.to_i, :start) if n =~ /[1-9]/
          c += 1
        end
        r += 1
      end
    end

    # Returns true if the cell can be set to the value.
    #
    # @param r [Fixnum] one-based row
    # @param c [Fixnum] one-based column
    # @param n [Fixnum] one-based cell value
    # @return [Boolean]
    def possible?(r, c, n)
      cell(r, c).possible? n
    end

    # Returns true if the cell has been set to a value.
    #
    # @param r [Fixnum] one-based row
    # @param c [Fixnum] one-based column
    # @return [Boolean]
    def known?(r, c)
      cell(r, c).known?
    end

    # Sets the cell to the value for the given reason (omit when playing).
    #
    # @param r [Fixnum] one-based row
    # @param c [Fixnum] one-based column
    # @param n [Fixnum] one-based cell value
    # @param reason [Symbol] one of :start, :choose, or :force
    # @return [void]
    #
    # @raise [RuntimeError] if the cell is already set
    # @raise [RuntimeError] if the value is not possible for the cell
    def set(r, c, n, reason = :choose)
      cell(r, c).set(n, reason)
    end

    # Iterates over each cell on the board from left-to-right and top-to-bottom.
    #
    # Used by the inspector.
    #
    # @yieldparam cell [Cell]
    # @return [void]
    def each_cell
      @cells.each { |row| row.each { |cell| yield cell } }
    end

    def to_s
      @cells.map { |row| row * "" } * "\n"
    end

    # Creates an inspector for this board.
    #
    # @return [Inspector] doesn't yet fully implement the output available in this class
    def inspector
      Inspector.new self
    end

    private

    def cell(r, c)
      @cells[r - 1][c - 1]
    end

    def row_cells(r)
      Array.new(9) { |c| @cells[r][c] }
    end

    def col_cells(c)
      Array.new(9) { |r| @cells[r][c] }
    end

    def box_cells(b)
      box_r = b % 3 * 3
      box_c = b / 3 * 3
      Array.new(9) { |i| @cells[box_r + i % 3][box_c + i / 3] }
    end

    def connect(group, cells)
      cells.each { |cell| cell.join group }
    end

    ### Debugging

    public

    def inspect
      "Board State\n#{to_s}"
    end

    def debug
      debug_cells
      debug_rows
      debug_cols
      debug_boxes
    end

    def debug_cells
      puts "Cell Possibilities"
      puts @cells.map { |row| row.map { |cell| cell.debug } * " " } * "\n"
    end

    def debug_rows
      puts "Row Possibilities"
      debug_group(@rows)
    end

    def debug_cols
      puts "Column Possibilities"
      debug_group(@cols)
    end

    def debug_boxes
      puts "Box Possibilities"
      debug_group(@boxes)
    end

    private

    def debug_group(groups)
      puts groups.map { |group| group.debug } * "\n"
    end
  end
end

Group.rb

require 'set'

module Sudoku

  # Tracks the cells that make up a single row, column, or 3x3 box, which are still available
  # to be set to which values, and the values still left to be set somewhere in the group.
  class Group

    # Creates an empty group of the given type.
    #
    # @param type [Symbol] either :row, :col, or :box
    # @param index [Fixnum] zero-based index
    def initialize(type, index)
      @type, @index = type, index
      @id = "#{type}-#{index + 1}"
      @knowns = {}
      @possibles = Hash[(1..9).map { |n| [n, Set.new] }]
    end

    attr_reader :id, :type, :index

    # Makes the cell available for the given value or all values.
    #
    # @param cell [Cell] the cell that can be set to the value
    # @param n [Fixnum or nil] omit to make the cell available for every value
    # @return [void]
    def available(cell, n = nil)
      if n
        @possibles[n] << cell
      else
        @possibles.each_value { |cells| cells << cell }
      end
    end

    # Makes the cell unavailable to receive the value.
    #
    # If the value has only one cell possibility left, it is set to the value.
    #
    # @param cell [Cell] the cell that can no longer be set to the value
    # @param n [Fixnum] one-based cell value
    # @return [void]
    def unavailable(cell, n)
      # puts "%s - unavail: %s -> %d: %s" % [id, cell.id, n, debug]
      cells = @possibles[n]
      cells.delete cell
      if cells.size == 1 and !known? n
        cells.take(1).first.set n, :force
      end
    end

    # Returns true if a cell in this group has been set to the value.
    #
    # @param n [Fixnum] one-based cell value
    # @return [Boolean] true if set; false if still available for this group
    def known?(n)
      !!@knowns[n]
    end

    # Returns true if no cell in this group has been set to the value.
    #
    # @param n [Fixnum] one-based cell value
    # @return [Boolean] false if none set; true if still available for this group
    def possible?(n)
      !@knowns[n]
    end

    # Returns the number of cells that can still be set to the value.
    #
    # @param n [Fixnum] one-based cell value
    # @return [Fixnum]
    def num_possible(n)
      @possibles[n].size
    end

    # Stores that the cell has been set to the value and removes the value as a possible from other cells.
    #
    # @param cell [Cell] the cell that was just set
    # @param n [Fixnum] one-based cell value
    # @return [void]
    def known(cell, n)
      # puts "%s - known  : %s -> %d: %s" % [id, cell.id, n, debug]
      raise "#{@knowns[n]} in #{@id} already known for #{n}" if known? n
      raise "#{cell} in #{@id} not possible for #{n}" unless @possibles[n].delete? cell
      @knowns[n] = cell
      @possibles[n].each { |possible| possible.unavailable self, n }
    end

    # Returns a string denoting which values are still possible and in how many cells.
    #
    # @return [String] first group: possible values are as-is; not possible values are dots;
    #                  second group: number of available cells per cell value
    def debug
      ((1..9).map { |n| possible?(n) ? n : "." } * "") + " " + ((1..9).map { |n| num_possible n } * "")
    end
  end
end

Cell.rb

require 'set'

module Sudoku

  # Models a single cell that tracks the possible values to which it may be set
  # and the current value if already set.
  class Cell

    # Creates an empty cell at the given row and column.
    #
    # @param r [Fixnum] zero-based row number
    # @param c [Fixnum] zero-based column number
    def initialize(r, c)
      @r, @c = r, c
      @id = "(#{r + 1},#{c + 1})"
      @initial = @current = nil
      @possibles = (1..9).to_set
      @groups = []
    end

    # Joins the given group and makes this cell available for all numbers.
    #
    # @param group [Group] contains this cell
    # @return [void]
    def join(group)
      @groups << group
      group.available self
    end

    attr_reader :id, :r, :c, :initial, :current

    # Returns true if this cell can be set to the value.
    #
    # @param n [Fixnum] one-based cell value
    # @return [Boolean]
    def possible?(n)
      !known? and @possibles.include? n
    end

    # Iterates over each value this cell can be set to.
    #
    # @yieldparam n [Fixnum] one-based cell value
    # @return [void]
    def each_possible
      @possibles.each { |n| yield n } if !known?
    end

    # Returns true if this cell has been set to a value.
    #
    # @return [Boolean]
    def known?
      !!@current
    end

    # Removes a value from this cell's set of possibilities.
    #
    # If this leaves the cell with but one possibility, the cell is set to it.
    #
    # @param n [Fixnum] one-based cell value
    # @return [void]
    #
    # @raise [RuntimeError] if n is the only possibility left
    def known(n)
      if possible? n
        raise "No possibilities left for cell #{@id}" if @possibles.size == 1
        @possibles.delete n
        set(@possibles.take(1).first, :force) if @possibles.size == 1
      end
    end

    # Notifies this cell's groups that the value is no longer possible for this cell.
    #
    # @param n [Fixnum] one-based cell value
    # @return [void]
    #
    # @todo Merge with #known.
    def unavailable(group, n)
      # puts "      - unavail: %s -> %d: %s           from %s" % [id, n, debug, group.id]
      @groups.each { |possible| possible.unavailable self, n }
      known n
    end

    # Sets this cell to the value and notifies each group.
    #
    # @param n [Fixnum] one-based cell value
    # @param reason [Symbol] either :setup, :choose, or :force
    # @return [void]
    #
    # @raise [RuntimeError] if this cell is already set
    # @raise [RuntimeError] if the value is not possible for this cell
    def set(n, reason)
      puts "      - %-7s: %s -> %d: %s" % [reason, id, n, debug]
      @initial = n if reason == :initial
      return if @current == n
      raise "cannot change #{@id} from #{@current} to #{n}" if @current
      raise "Cannot set #{@id} to #{n}" if !possible? n
      @possibles.delete n
      @current = n
      @groups.each do |group|
        group.known self, n
        @possibles.each { |n| group.unavailable self, n }
      end
      @possibles.clear
    end

    # Returns the current value of this cell if set or a dot if not.
    #
    # @return [String] single digit or a dot
    def to_s
      (@current || ".").to_s
    end

    # Returns a string denoting which values are still possible.
    #
    # @return [String] possible values are as-is; not possible values are dots
    def debug
      (1..9).map { |n| possible?(n) ? n : "." } * ""
    end
  end
end

Test.rb

require 'Board'

module Sudoku

  # Creates several test puzzles from easy to evil.
  #
  # @see http://www.websudoku.com/
  module Test

    def self.easy
      self.test <<-PUZZLE
        ..38.4.5.
        84......7
        6....29..
        .18.5..9.
        2.6.7.4.5
        .3..4.86.
        ..51....8
        1......79
        .6.5.7.2.
      PUZZLE
    end

    def self.medium
      self.test <<-PUZZLE
        .5.....7.
        9428.....
        ....6...5
        48..765..
        .93...78.
        ..598..14
        7...4....
        .....8193
        .1.....4.
      PUZZLE
    end

    def self.hard
      self.test <<-PUZZLE
        .....5.8.
        .8....5.1
        ..5.12.69
        ......17.
        .2.....3.
        .19......
        15.34.7..
        8.7....1.
        .6.9.....
      PUZZLE
    end

    def self.evil # http://www.websudoku.com/?level=4&set_id=6247568390
      self.test <<-PUZZLE
        .4.2.....
        .....13.7
        ..5...91.
        .8..13...
        ..3.6.5..
        ...72..4.
        .12...4..
        8.69.....
        .....2.3.
      PUZZLE
    end

    def self.test(puzzle)
      puts "Initial Puzzle"
      puts puzzle

      b = Board.new puzzle
      b.debug
      b.inspector.cells
      b
    end
  end
end

Inspector.rb

module Sudoku

  # Assists debugging the internal state of the board.
  class Inspector

    # Stores the board for inspection
    #
    # @param board [Board]
    def initialize(board)
      @board = board
    end

    # Displays the current cell values.
    def current
      rows = []
      row = nil
      @board.each_cell do |cell|
        if cell.c == 0
          row = ""
          rows << "" if cell.r != 0 && cell.r % 3 == 0
        end
        row += " " if cell.c != 0 && cell.c % 3 == 0
        row += cell.to_s
        rows << row if cell.c == 8
      end
      puts "Current Puzzle"
      puts
      puts rows * "\n"
    end

    # Displays the available values for each cell.
    def cells
      grid = ([["... " * 9] * 3, ""].flatten * 9).map(&:clone)
      @board.each_cell do |cell|
        r, c = 4 * cell.r, 4 * cell.c
        cell.each_possible do |n|
          grid[r + (n - 1) / 3][c + (n - 1) % 3] = n.to_s
        end
      end
      puts "Cell Possibilities"
    puts
      puts grid * "\n"
    end

    def inspect
      "Inspector"
    end
  end
end

Sample Test Run

-> require 'Test'
-> Sudoku::Test::easy

Initial Puzzle

..38.4.5.
84......7
6....29..
.18.5..9.
2.6.7.4.5
.3..4.86.
..51....8
1......79
.6.5.7.2.

start  : (1,3) -> 3: 123456789
start  : (1,4) -> 8: 12.456789
start  : (1,6) -> 4: 12.4567.9
start  : (1,8) -> 5: 12..567.9
start  : (2,1) -> 8: 12.456789
start  : (2,2) -> 4: 12.4567.9
start  : (2,9) -> 7: 123..67.9
start  : (3,1) -> 6: 12..567.9
start  : (3,6) -> 2: 123.5.7.9
start  : (3,7) -> 9: 1.34...89
start  : (4,2) -> 1: 123.56789
start  : (4,3) -> 8: .2.456789
start  : (4,5) -> 5: .234567.9
start  : (4,8) -> 9: .234.67.9
start  : (5,1) -> 2: .2345.7.9
start  : (5,3) -> 6: ...4567.9
start  : (5,5) -> 7: 1.34..789
force  : (3,4) -> 7: 1.3.5.7..
force  : (3,2) -> 5: ....5....
force  : (3,3) -> 1: 1........
force  : (3,5) -> 3: ..3......
start  : (5,7) -> 4: 1.345..8.
force  : (5,9) -> 5: 1.3.5..8.
start  : (5,9) -> 5: .........
start  : (6,2) -> 3: ..3...7.9
force  : (5,2) -> 9: ........9
start  : (6,5) -> 4: 12.4.6.89
force  : (4,1) -> 4: ...4..7..
force  : (4,7) -> 7: .23..67..
force  : (6,5) -> 4: .........
start  : (6,7) -> 8: 12...6.8.
force  : (5,6) -> 8: 1.3....8.
force  : (6,7) -> 8: .........
start  : (6,8) -> 6: 12...6...
start  : (7,3) -> 5: .2.45.7.9
force  : (6,1) -> 5: ....5.7..
force  : (6,3) -> 7: ....5.7..
force  : (6,1) -> 5: .........
force  : (7,3) -> 5: .........
start  : (7,4) -> 1: 1234.6..9
force  : (5,8) -> 1: 1.3......
force  : (5,4) -> 3: 1.3......
force  : (4,9) -> 3: .23......
force  : (4,4) -> 2: .2...6...
force  : (4,6) -> 6: ..3..6...
force  : (4,4) -> 2: .........
force  : (6,9) -> 2: 12.......
force  : (5,8) -> 1: .........
force  : (6,6) -> 1: 1.......9
force  : (6,4) -> 9: 1.......9
force  : (6,6) -> 1: .........
start  : (7,9) -> 8: ...4.6.89
force  : (7,8) -> 4: .234..7..
force  : (3,9) -> 4: ...4...8.
force  : (3,8) -> 8: ...4...8.
force  : (3,9) -> 4: .........
force  : (7,8) -> 4: .........
force  : (7,9) -> 8: .........
start  : (8,1) -> 1: 1.3...7.9
force  : (8,1) -> 1: .........
start  : (8,8) -> 7: .23...7..
force  : (8,8) -> 7: .........
start  : (8,9) -> 9: .....6..9
force  : (8,9) -> 9: .........
start  : (9,2) -> 6: .2...678.
force  : (9,2) -> 6: .........
force  : (8,2) -> 8: .2.....8.
force  : (9,5) -> 8: .2...6.89
force  : (9,5) -> 8: .........
force  : (8,2) -> 8: .........
force  : (1,9) -> 6: 1....6...
force  : (9,9) -> 1: 1....6...
force  : (9,9) -> 1: .........
start  : (9,4) -> 5: ...45....
force  : (2,6) -> 5: ....5...9
force  : (2,4) -> 6: .....6...
force  : (2,4) -> 6: .........
force  : (8,6) -> 3: ..3......
force  : (8,6) -> 3: .........
force  : (9,4) -> 5: .........
force  : (8,7) -> 5: .2..56...
force  : (9,4) -> 5: .........
force  : (8,7) -> 5: .........
force  : (7,7) -> 6: .23..6...
force  : (8,5) -> 6: .2...6...
force  : (7,5) -> 2: .2...6..9
force  : (8,5) -> 6: .........
force  : (1,2) -> 2: .2....7..
force  : (1,7) -> 1: 1........
force  : (2,5) -> 1: 1.......9
force  : (1,7) -> 1: .........
force  : (1,7) -> 1: .........
force  : (2,3) -> 9: .2......9
force  : (1,5) -> 9: 1.......9
force  : (2,3) -> 9: .........
force  : (1,5) -> 9: .........
force  : (2,5) -> 1: .........
force  : (2,5) -> 1: .........
force  : (1,1) -> 7: ......7..
force  : (1,1) -> 7: .........
force  : (7,2) -> 7: .2....7..
force  : (1,2) -> 2: .........
force  : (1,1) -> 7: .........
force  : (9,6) -> 7: ......7.9
force  : (9,6) -> 7: .........
force  : (7,2) -> 7: .........
force  : (7,6) -> 9: ........9
force  : (7,6) -> 9: .........
force  : (9,1) -> 9: ..3...7.9
force  : (7,1) -> 3: ..3.....9
force  : (1,1) -> 7: .........
force  : (2,3) -> 9: .........
force  : (9,1) -> 9: .........
force  : (9,6) -> 7: .........
force  : (1,2) -> 2: .........
force  : (7,7) -> 6: .........
force  : (7,7) -> 6: .........
force  : (8,3) -> 2: .2.4.....
force  : (8,3) -> 2: .........
force  : (9,3) -> 4: ...4.....
force  : (9,3) -> 4: .........
force  : (8,4) -> 4: ...45....
force  : (8,4) -> 4: .........
force  : (9,3) -> 4: .........
force  : (9,4) -> 5: .........
start  : (9,6) -> 7: .........
start  : (9,8) -> 2: .23......
force  : (2,7) -> 2: .23......
force  : (2,8) -> 3: .23......
force  : (9,7) -> 3: .23......
force  : (2,7) -> 2: .........
force  : (2,8) -> 3: .........
force  : (9,7) -> 3: .........
force  : (9,8) -> 2: .........
force  : (9,8) -> 2: .........
force  : (2,7) -> 2: .........

Board State

723894156
849615237
651732984
418256793
296378415
537941862
375129648
182463579
964587321

What kind of feedback are you looking for? For example, are you looking for feedback on readability, performance, etc.? — Kenrick Chien, Dec 19 '13 at 16:17
@KenrickChien - Please see the update near the top of my question. — David Harkness, Dec 19 '13 at 18:16

Flambino · Accepted Answer · 2013-12-21 00:41:35Z

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.

def cell[](row, column)
  cells[row - 1][column - 1]
end

which'll let you call, say, board.cell[row, column].known?.

Accessing a cell directly like that is of course a departure from your current practice of proxying a lot of the Cell class's methods directly in Board (e.g. Board#known?(r, c)).
The reasons for proxying are much the same as for accessors: Add some implementation flexibility and provide a flat API. However, here it smells like repetition to me. So I'd probably prefer just getting a cell, and chaining off of that like in the example above. It just seems cleaner and more expressive to me, than replicating all its methods on Board. But arguments can be made either way.

(More worrisome are perhaps the circular references and side-effects you create between a cell and the groups it belongs to, but again, I'll leave the specifics of the solver's structure alone)

Oh, man. I've only been looking at Board but you've got lots of other classes :)
I'm out of time, though, so here are what I found while skimming through stuff:

More methods, just in general, to break up the long'ish ones you have here and there.
I'd remove references to the Inspector class from Board; the board doesn't need to know how it's being presented.
Or, I'd roll the functionality into board itself; it can be shortened considerably, so it won't seem like a burden. For instance, the Inspector#current method could be:
```
def current
  puts "Current Puzzle"
  puts cells.map { |row|
    row.map(&:to_s).each_slice(3).to_a.map(&:join).join(" ")
  }.each_slice(3).map { |slice| slice.join("\n") }.join("\n\n")
end
```
Ok, that's maybe a bit crazy, but still: gotta love Array/Enumerable stuff :)
Lastly: Ruby files are always named in lowercase. Completely irrelevant to the code of course (apart from the require statements), but you don't want to stand out as the "Java guy", right? :)

Hope I gave you some worthwhile ideas.

Lots of excellent advice here. Every language I learn leaves a trace in how I code elsewhere; I take no offense. I avoid using other libraries like testing frameworks while starting so I'm forced to use I/O for debugging and other low-evel techniques. Many of your refactoring suggestions are spot on and would apply in other languages. — David Harkness, Dec 22 '13 at 16:48
The facade methods in Board are there to hide the internal structure. However, if I expose Cell directly, without package-private methods, how can I hide its "internal" methods that Group needs to call? — David Harkness, Dec 22 '13 at 16:56
BTW, very nice use of scan there. I gave up looking for a way to get automatic loop counters when there was no each_char_with_index. :) — David Harkness, Dec 22 '13 at 16:59
@DavidHarkness I'm glad you found it helpful! And please don't think I took any offense! :) Good point about basic I/O vs. tests. There's a lot of tooling for Ruby, so it's kinda daunting at first, and probably not where you should jump in. But do jump in at some point :) — Flambino, Dec 22 '13 at 17:05
@DavidHarkness re your 2nd comment: Ruby's a lot more relaxed regarding access to stuff. For instance, you can call private methods from anywhere if you want, and you can monkeypatch anything, redeclare classes, whatever. It's very flexible and powerful, but also totally different from other languages. Here, there's also a design choice: Do you want Board to be the only API, or do you want to expose the building blocks (Cell, Group) too? Ruby leans toward the latter, since you can always get at the internals anyway, so there's little point in worrying too much — Flambino, Dec 22 '13 at 17:15

asked	3 months ago
viewed	232 times
active	3 months ago

current community

your communities

more stack exchange communities

Ruby Sudoku solver

1 Answer

Your Answer

Not the answer you're looking for? Browse other questions tagged ruby weekend-challenge sudoku or ask your own question.

Hot Network Questions

current community

your communities

more stack exchange communities

Ruby Sudoku solver

1 Answer

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged ruby weekend-challenge sudoku or ask your own question.

Related

Hot Network Questions