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