Fork me on GitHub
#code-reviews
<
2015-09-03
>
sooheon12:09:40

I’m trying to learn by translating some of Peter Norvig’s python (http://norvig.com/sudoku.html) to clojure. Could anyone point out what I could do better here?

(ns sudoku.core
  (:require [clojure.set :refer [difference]]))

(defn cross
  "Cross product of elements in A and elements in B."
  [a-list b-list]
  (for [a a-list
        b b-list]
    (str a b)))

(def rows "ABCDEFGHI")

(def cols "123456789")

(def squares
  (cross rows cols))

(def unitlist
  (concat (for [c cols] (->> (cross rows (repeat 9 c)) set vec sort))
          (for [r rows] (->> (cross (repeat 9 r) cols) set vec sort))
          (for [rs ["ABC" "DEF" "GHI"]
                cs ["123" "456" "789"]]
            (->> (cross rs cs) vec))))

(def units
  "Hash-map with squares as keys and the units of each square as a list of
  lists."
  (into {} (for [s squares]
             [s (for [u unitlist :when (some #{s} u)]
                  u)])))

(def peers
  "Hash-map associating each square and its peers."
  (into {} (for [s squares]
             [s (-> (units s)
                    flatten
                    set
                    (difference #{s}))])))

meow13:09:18

@sooheon: ah, interesting. I am quite familiar with Norvig's sudoku code as I rewrote a variation of it to go with a QML Sudoku app I wrote.

sooheon13:09:33

@meow: Hey it’s been a while! Spent the summer doing other stuff but back to learning clojure again :) I also have the test code

(deftest sudoku-setup
  (testing "Basic unit tests for the board."
    (is (= 81 (count squares)))
    (is (= 27 (count unitlist)))
    (is (every? #(= 3 %) (for [s squares]
                           (count (units s)))))
    (is (every? #(= 20 %) (for [s squares]
                            (count (peers s)))))
    (is (= [["A2" "B2" "C2" "D2" "E2" "F2" "G2" "H2" "I2"]
            ["C1" "C2" "C3" "C4" "C5" "C6" "C7" "C8" "C9"]
            ["A1" "A2" "A3" "B1" "B2" "B3" "C1" "C2" "C3"]]
           (units "C2")))
    (is (= #{"I2" "A1" "E2" "A3" "C1" "B1" "B3" "C3" "C7"
             "A2" "C6" "G2" "C5" "F2" "B2" "C8" "C4" "H2"
             "D2" "C9"}
           (peers "C2")))))

sooheon13:09:08

Nice, I can look through both yours and Norvigs version

meow13:09:05

Norvig's version is slightly flawed in that it will produce puzzles that can have more than one valid solution, which is usually NOT what you want from a sudoku generator

meow13:09:18

My version fixed that

meow13:09:36

But my code reflects my then infatuation with OO code.

meow13:09:54

I hesitate to really look at it because I'm sure I'll hate it now.

meow13:09:34

The QML app I created was quite nice, if I do say so myself. Unfortunately, it convinced me that QML was not what I wanted to work with long term and then I moved on to Clojure and I am so glad that I did.

meow13:09:08

I wrote a fair amount of unit tests for that Python code: https://github.com/pkobrien/sudoku/blob/master/tests/test_sudoku.py

sooheon13:09:18

Wow this is all very helpful, all the differences/improvements to Norvig’s code will be great to study.

sooheon13:09:23

Anything about my clojure code to critique? I wonder if there’s any better way to do the (is (every? #(= 3 %) (foo))) test...

meow13:09:27

I haven't done that much unit testing in clojure (something I'm just now working on) but those tests look good to me.

potetm13:09:54

I would tend to do (every? #{3} ..).

potetm13:09:39

But there are some tradeoffs there I presume. I would assume (partial = 3) to be more performant, but, in my mind, the “set of 3” allows you to think more about data and less about the particular operation you want to perform (i.e. =).

sooheon14:09:04

@potetm: Thanks, the set notation makes for much more readable reports than anonymous functions when the test fails as well.