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. 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. It solves sudoku puzzles. (What pleasure do people get by doing these in their heads?!?) Cheers, David -------------------------------- David F. Place mailto:d@vidplace.com
On 3/22/06, David F. Place
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. 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
Try
cellIndex r c = 3*(r `div` 3) + c `div` 3
It's much much shorter and should produce the same results.
It solves sudoku puzzles. (What pleasure do people get by doing these in their heads?!?)
They are probably asking the same question: why take hours to write a program to do it when with my mad sudoku solving skills I can solve it in X seconds? My roommate is like this. Cheers, Jared. -- http://www.updike.org/~jared/ reverse ")-:"
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. 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.
sudoku :: Sudoku -> IO () sudoku s = ((mapM_ putStrLn) . (check s) . (take 1) . solveSudoku) s
check puzzle [] = [showSudoku puzzle,"No solutions."] check puzzle [solution] | solution `solves` puzzle = ["Puzzle:",showSudoku puzzle,"Solution:",showSudoku solution] | otherwise = ["Program Error. Incorrect Solution!"]
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> 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). FYI, solveSudoku has a bug; if you enter an invalid puzzle it will return non-solutions.
It solves sudoku puzzles. (What pleasure do people get by doing these in their heads?!?)
I have no idea. Rob Dockins Speak softly and drive a Sherman tank. Laugh hard; it's a long way to the bank. -- TMBG
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
Thanks for your helpful suggestions. I took them to heart and incorporated many of them in a new version. -------------------------------- David F. Place mailto:d@vidplace.com
participants (4)
-
Chris Kuklewicz -
David F. Place -
Jared Updike -
Robert Dockins