n-puzzle in Clojurescript

Question

I'm in the process of creating a n-puzzle solver in Clojurescript. I have the basic model up and running and can move tiles on the screen. I plan to implement A* or such an algorithm to solve a given puzzle. Before I do that, I welcome any general comments for improving the quality of the code right now.

I am just pasting the most important files only here. The entire repo is here

puzzle.cljs

 (ns npuzzle.puzzle
 (:require [cljs.test :refer-macros [deftest testing is run-tests]]
 [clojure.zip :as zip]))
 (defn get-tiles [puzzle] (nth puzzle 0))
 (defn get-size [puzzle] (nth puzzle 1))
 (defn set-tiles [puzzle tiles] (assoc puzzle 0 tiles))
 (defn make-puzzle
 ([size]
 [(conj (vec (range 1 (* size size))) :space) size])
 ([tiles size]
 [tiles size]))
 (defn index->row-col [puzzle index]
 (let [size (get-size puzzle)]
 [(quot index size) (mod index size)]))
 (defn row-col->index [puzzle [row col]]
 (let [size (get-size puzzle)]
 (+ (* row size) col)))
 (defn get-tile [puzzle [row col]]
 ((get-tiles puzzle) (+ (* row (get-size puzzle)) col)))
 (defn get-index-of-space [puzzle]
 (.indexOf (get-tiles puzzle) :space))
 (defn get-space-coords [puzzle]
 (index->row-col puzzle (get-index-of-space puzzle)))
 (defn is-space? [puzzle coords]
 (= :space (get-tile puzzle coords)))
 (defn valid-coords? [puzzle [row col]]
 (let [size (get-size puzzle)]
 (and (>= row 0) (>= col 0)
 (< row size) (< col size))))
 (defn get-adjacent-coords [puzzle [row col]]
 (->> '([0 1] [-1 0] [0 -1] [1 0]) ;;'
 (map (fn [[dr dc]]
 [(+ row dr) (+ col dc)]))
 (filter #(valid-coords? puzzle %))))
 (defn inversions [puzzle]
 (let [size (get-size puzzle)
 tiles (get-tiles puzzle)
 inversions-fn
 (fn [remaining-tiles found-tiles inversions]
 (if-let [i (first remaining-tiles)]
 (recur (rest remaining-tiles)
 (conj found-tiles i)
 (+ inversions (- (dec i) (count (filter found-tiles (range i))))))
 inversions))]
 (inversions-fn (remove #{:space} tiles) #{} 0)))
 (defn is-solvable? [puzzle]
 (let [inv-cnt (inversions puzzle)
 size (get-size puzzle)
 [row col] (get-space-coords puzzle)]
 (if (odd? size)
 (even? inv-cnt)
 (if (even? (- size row))
 (odd? inv-cnt)
 (even? inv-cnt)))))
 (defn can-move? [puzzle coords]
 ;;Check if any one of the adjacent tiles is the space.
 (->> (get-adjacent-coords puzzle coords)
 (filter #(is-space? puzzle %))
 (first)))
 (defn swap-tiles [puzzle coord1 coord2]
 (let [index1 (row-col->index puzzle coord1)
 index2 (row-col->index puzzle coord2)
 tile1 (get-tile puzzle coord1)
 tile2 (get-tile puzzle coord2)
 tiles (get-tiles puzzle)]
 (set-tiles puzzle
 (-> tiles
 (assoc index1 tile2)
 (assoc index2 tile1)))))
 (defn move [puzzle coords]
 (if (can-move? puzzle coords)
 (swap-tiles puzzle coords (get-space-coords puzzle))
 puzzle))
 (defn random-move [puzzle]
 (let [space-coords (get-space-coords puzzle)
 movable-tiles (get-adjacent-coords puzzle space-coords)
 random-movable-tile (nth movable-tiles (rand-int (count movable-tiles)))]
 (move puzzle random-movable-tile)))
 (defn shuffle-puzzle
 "Shuffles by moving a random steps"
 ([puzzle]
 (let [num-moves (+ 100 (rand 200))]
 (shuffle-puzzle puzzle num-moves)))
 ([puzzle num-moves]
 (if (> num-moves 0)
 (recur (random-move puzzle) (dec num-moves))
 puzzle)))

puzzle_test.cljs

(ns npuzzle.puzzle-test
 (:require [cljs.test :refer-macros [deftest testing is run-tests]]
 [npuzzle.puzzle :as pzl]))
(deftest puzzle-tests
 (let [p1 (pzl/make-puzzle 3)]
 (testing "make-puzzle"
 (is (= [1 2 3 4 5 6 7 8 :space] (pzl/get-tiles p1)))
 (is (= 3 (pzl/get-size p1))))
 (testing "get-tile"
 (is (= 1 (pzl/get-tile p1 [0 0])))
 (is (= 2 (pzl/get-tile p1 [0 1])))
 (is (= 5 (pzl/get-tile p1 [1 1]))))
 (testing "get-index-of-space"
 (is (= 8 (pzl/get-index-of-space p1))))
 (testing "get-space-coords"
 (is (= [2 2] (pzl/get-space-coords p1))))
 (testing "is-space?"
 (is (pzl/is-space? p1 [2 2]))
 (is (not (pzl/is-space? p1 [2 1]))))
 (testing "index->row-col"
 (is (= [1 1] (pzl/index->row-col p1 4)))
 (is (= [2 3] (pzl/index->row-col (pzl/make-puzzle 5) 13))))
 (testing "row-col->index"
 (is (= 4 (pzl/row-col->index p1 [1 1])))
 (is (= 13 (pzl/row-col->index (pzl/make-puzzle 5) [2 3]))))
 (testing "valid-coords?"
 (is (pzl/valid-coords? p1 [1 1]))
 (is (not (pzl/valid-coords? p1 [2 3]))))
 (testing "get-adjacent-coords"
 (is (= #{[0 1] [1 0] [1 2] [2 1]} (into #{} (pzl/get-adjacent-coords p1 [1 1]))))
 (is (= #{[0 1] [1 0]} (into #{} (pzl/get-adjacent-coords p1 [0 0])))))
 (testing "can-move?"
 (is (pzl/can-move? p1 [1 2]))
 (is (pzl/can-move? p1 [2 1]))
 (is (not (pzl/can-move? p1 [1 1]))))
 (testing "move"
 (is (= [1 2 3 4 5 :space 7 8 6] (pzl/get-tiles (pzl/move p1 [1 2]))))
 (is (= [1 2 3 4 :space 5 7 8 6] (pzl/get-tiles (pzl/move (pzl/move p1 [1 2]) [1 1]))))
 (is (= [1 2 3 4 5 6 7 8 :space] (pzl/get-tiles (pzl/move p1 [0 1])))))
 (testing "inversions"
 (is (= 10 (pzl/inversions (pzl/make-puzzle [1 8 2 :space 4 3 7 6 5] 3))))
 (is (= 41 (pzl/inversions (pzl/make-puzzle [13 2 10 3 1 12 8 4 5 :space 9 6 15 14 11 7] 4))))
 (is (= 62 (pzl/inversions (pzl/make-puzzle [6 13 7 10 8 9 11 :space 15 2 12 5 14 3 1 4] 4))))
 (is (= 56 (pzl/inversions (pzl/make-puzzle [3 9 1 15 14 11 4 6 13 :space 10 12 2 7 8 5] 4)))))
 (testing "is-solvable?"
 (is (pzl/is-solvable? (pzl/make-puzzle [1 8 2 :space 4 3 7 6 5] 3)))
 (is (pzl/is-solvable? (pzl/make-puzzle [13 2 10 3 1 12 8 4 5 :space 9 6 15 14 11 7] 4)))
 (is (pzl/is-solvable? (pzl/make-puzzle [6 13 7 10 8 9 11 :space 15 2 12 5 14 3 1 4] 4)))
 (is (not (pzl/is-solvable? (pzl/make-puzzle [3 9 1 15 14 11 4 6 13 :space 10 12 2 7 8 5] 4)))))
 (testing "shuffle-puzzle"
 (is (pzl/is-solvable? (pzl/shuffle-puzzle (pzl/make-puzzle 4))))
 (is (pzl/is-solvable? (pzl/shuffle-puzzle (pzl/make-puzzle 6))))
 (is (pzl/is-solvable? (pzl/shuffle-puzzle (pzl/make-puzzle 9))))
 (is (not= (pzl/shuffle-puzzle (pzl/make-puzzle 3)) (pzl/make-puzzle 3)))
 (is (not= (pzl/shuffle-puzzle (pzl/make-puzzle 8)) (pzl/make-puzzle 8))))))
(run-tests)

views.cljs

(ns npuzzle.views
 (:require
 [re-frame.core :as re-frame]
 [npuzzle.subs :as subs]
 [npuzzle.events :as ev]))
(defn- on-puzzle-size-changed [e]
 (re-frame/dispatch [::ev/change-puzzle-size (int (-> e .-target .-value))]))
(defn- on-tile-mouse-down [e]
 (let [idx (.getAttribute (-> e .-target) "data-tileindex")]
 (re-frame/dispatch [::ev/move-tile (int idx)])))
(defn- tile-div [idx tile]
 [:div {:style {:text-align :center
 :padding "15px"
 :font-size "20px"
 :border (if (not= :space tile) "1px solid black" :none)
 :background (if (not= :space tile) :lightblue nil)
 :height "25px"
 :width "25px"
 ;;cursor = default for disabling caret on hover over text
 :cursor :default}
 :onMouseDown on-tile-mouse-down
 :data-tileindex idx}
 (if (= :space tile)
 " "
 tile)])
(defn- tiles-container [puzzle size tile-fn]
 (into [:div {:style {:display :inline-grid
 :grid-template-columns (apply str (repeat size "auto "))
 :grid-gap "2px"
 :border "2px solid black"}}]
 (map-indexed
 (fn [idx tile]
 (tile-fn idx tile)) puzzle)))
(defn main-panel []
 (let [name (re-frame/subscribe [::subs/name])
 sizes (re-frame/subscribe [::subs/size-choices])
 current-size (re-frame/subscribe [::subs/puzzle-size])
 puzzle (re-frame/subscribe [::subs/puzzle])]
 [:div
 [:h1 "Hello to " @name]
 [:div
 {:style {:padding "10px"}}
 (into [:select {:name "puzzle-size"
 :on-change on-puzzle-size-changed
 :defaultValue @current-size}]
 (for [size @sizes]
 (into [:option {:value size} size])))]
 (tiles-container @puzzle @current-size tile-div)]))

Answer 1

I don't really see anything major. I also don't know Cljs (only Clj), so I apologize if a suggestion of mine doesn't apply to Cljs.

Just a few small things:

Personally, I don't like having function definitions all on one line. Increasingly, I'm even splitting def definitions over two lines. I find it generally helps readability. I'd change your definitions at the top to:

(defn get-tiles [puzzle]
 (nth puzzle 0))
(defn get-size [puzzle]
 (nth puzzle 1))
(defn set-tiles [puzzle tiles]
 (assoc puzzle 0 tiles))

---

And I don't see any sample data, but judging by the first two functions, you seem to be storing the puzzle as a vector "tuple" of [tiles size]. You may find that it's neater to store that as a map instead. Something like {:tiles [...], :size ...}. That gives you getters for free (as keywords), and makes the structure itself more self explanatory.

---

You're using :space as a sentinel value. You may find using a namespaced-qualified version (::space internally) is beneficial. I like using :: keywords when possible because IntelliJ gives better autocomplete suggestions when using namespaced keywords, so it's harder to cause typos.

---

valid-coords? can be simplified a bit by taking advantage of "comparison chaining":

(defn valid-coords? [puzzle [row col]]
 (let [size (get-size puzzle)]
 (and (< -1 row size)
 (< -1 col size))))

You could also use <= and dec size before comparing:

(defn valid-coords? [puzzle [row col]]
 (let [size (dec (get-size puzzle))]
 (and (<= 0 row size)
 (<= 0 col size))))

---

is-solvable? can have some of its nesting reduced using cond:

(defn is-solvable? [puzzle]
 (let [inv-cnt (inversions puzzle)
 size (get-size puzzle)
 [row col] (get-space-coords puzzle)]
 (cond
 (odd? size) (even? inv-cnt)
 (even? (- size row)) (odd? inv-cnt)
 :else (even? inv-cnt))))

I go back and forth between putting the conditions and bodies on the same and separate lines. In this case though, everything is short enough that readability is fine. If I spaced this out like I normally would:

(defn is-solvable? [puzzle]
 (let [inv-cnt (inversions puzzle)
 size (get-size puzzle)
 [row col] (get-space-coords puzzle)]
 (cond
 (odd? size)
 (even? inv-cnt)
 (even? (- size row))
 (odd? inv-cnt)
 :else
 (even? inv-cnt))))

It becomes huge, although I do think the readability is better in the second version.

---

(if (> num-moves 0) can alternatively be written as (if (pos? num-moves).

---

A lot of your tests have a series of is checks that are all basically checking the same thing, just with different data:

(testing "get-tile"
 (is (= 1 (pzl/get-tile p1 [0 0])))
 (is (= 2 (pzl/get-tile p1 [0 1])))
 (is (= 5 (pzl/get-tile p1 [1 1]))))

This can be simplified using are:

(testing "get-tile"
 (are [result coord] (= result (pzl/get-tile p1 coord))
 1 [0 0]
 2 [0 1]
 5 [1 1]))

That gets rid of a lot of explicit duplicate calls and noise.

• nakiya: Thanks for all the suggestions. I will incorporate these in...