[Haskell-cafe] Code Review: Sudoku solver

[Chris Kuklewicz]

Robert Dockins wrote:
> 
> On Mar 22, 2006, at 2:16 PM, David F. Place wrote:
> 
>> Hi All,
>>
>> I really appreciate all the help I received when I asked you to
>> critique my PrefixMap module a few weeks ago.  I think I am making
>> good progress in correcting the "lisp" in my Haskell programming. 

The style of the code and choice of names is good.

>> I'll be very grateful to anyone who can take a glance at the attached
>> short program and say if any unidiomatic usages pop out.
> 
> 
> That '(check s) . (take 1)' bit looks a little odd to me.  I would
> simply have written 'check' to match like:
> 
> check puzzle [] = <failure case>
> check puzzle (solution : _ ) = <success case>
> 

A simpler version of replace:

replace :: Sudoku -> Int -> Int -> Int -> Sudoku
replace s r c x =
  let (above,row:below) = splitAt r s
      (left,_:right) = splitAt c row
  in above++((left++(x:right)):below)

And a simpler version of toList in keeping with your style:

toList :: Set -> [Int]
toList i = concatMap f [9,8..1]
    where
      f b = if testBit i b then [b] else []

(The above is also a little less prone to off-by-one errors since testBit is the
opposite of setBit)

> 
> Also, I like to put off doing IO as long as possible, so I'd probably
> have 'sodoku' return a String or [String] and move the putStr into
> main.  Its an easy way to make your code more reusable.
> 
> Also, your parser is pretty hackish (but I suspect you knew that already).
> 
A minimal change to the parser that still does no sanity checking but may be a
little more robust is

import Data.Char

readSudoku :: [String] -> String -> Sudoku
readSudoku ["line"] input =
    takeBy 9 $ map digitToInt $ filter isDigit $ head $ lines input
readSudoku _ input =
    map (map digitToInt . filter isDigit) $ lines input