4
\$\begingroup\$

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)]))
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for all the suggestions. I will incorporate these in... \$\endgroup\$
    – nakiya
    Apr 10, 2019 at 8:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.