Program follows sequence of instructions to arrive at destination

Question

I'm warming up with Clojure to try my hand at the advent of code challenge that starts in December. This is my attempt at the first challenge from last year.

This is my first attempt at a non trivial program in clojure, so comments on any part of the program would be appreciated.

(defrecord Vector [x y])
(defrecord State [orientation position])
(def orientations {
 :north (Vector. 0 -1),
 :east (Vector. 1 0),
 :south (Vector. 0 1),
 :west (Vector. -1 0) })
;; Given a compass orientation
;; Return the new orientation after a left turn
(defn turnLeft [current_orientation]
 (case current_orientation
 :north :west
 :east :north
 :south :east
 :west :south))
;; Given a compass orientation
;; Return the new orientation after a right turn
(defn turnRight [current_orientation]
 (case current_orientation
 :north :east
 :east :south
 :south :west
 :west :north))
;; Given a position, orientation and distance
;; Return a new position after moving in that direction for that distance
(defn moveForward [current_position orientation distance]
 (let [direction_vector (orientation orientations)]
 (Vector.
 (+ (:x current_position) (* distance (:x direction_vector)))
 (+ (:y current_position) (* distance (:y direction_vector))))))
;; Given a state and an instruction
;; Return a new state
(defn act [state, instr]
 (case instr
 \L (State. (turnLeft (:orientation state)) (:position state))
 \R (State. (turnRight (:orientation state)) (:position state))
 (State. 
 (:orientation state) 
 (moveForward (:position state) (:orientation state) instr))))
;; Given a starting state and list of instructions
;; Return the final state
(defn doActions [state instrs]
 (if (empty? instrs)
 state
 (doActions (act state (first instrs)) (rest instrs))))
;; Instructions are a sequence of combined instructions as follows:
;; L13, R10, R1, L3, ...
;; Convert this into an alternating list 
;; of TurnLeft / TurnRight and Move instructions
(def instrs
 (->> "input.txt"
 slurp
 (re-seq #"[LR]|\d+")
 (map
 (fn [instr]
 (case instr
 "L" \L
 "R" \R
 (read-string instr))))))
(require '(java.lang.Math))
(let [finalstate (doActions (State. :north (Vector. 0 0)) instrs)]
 (+ 
 (Math/abs (get-in finalstate [:position :y])) 
 (Math/abs (get-in finalstate [:position :x]))))

Answer 1

I see a few things that can be improved on. I'll be commenting more on idiomatic Clojure, rather than the algorithm itself.

---

(Vector. 0 -1)

Using Java interop constructors for records usually isn't a great idea. I actually can't think of a time when I've ever used them. In a case like this, it seems fine, but lets say you try to apply the args instead:

(apply Vector. [0 -1]) ; Oops!
CompilerException java.lang.ClassNotFoundException: Vector.

Best just to get in the habit of using the auto-generated constructors for consistency, which can be used as you'd expect:

(->Vector 0 -1)
(apply ->Vector [0 -1])

---

Since maps ({}) implement IFn, they themselves can be used as functions. I would write your turnRight function as a map instead:

(def turnRight
 {:north :east
 :east :south
 :south :west
 :west :north})

Just like you did with orientations.

---

It seems to be generally more accepted to use "dash" naming instead of camelCase. I would instead call turnRight turn-right.

---

(defn doActions [state instrs]
 (if (empty? instrs)
 state
 (doActions (act state (first instrs)) (rest instrs))))

Use recur! doActions suffers from the typical recursion limitations. If the problem is too big, this will cause a SO! Whenever your recursion can be optimized, use recur to ensure that you don't get any StackOverflow surprises down the road. Your code could also make use of some destructuring to avoid needing to call first and rest explicitly. Using destructuring also makes it so you don't need to call empty?. I'd write that function as*:

(defn doActions [state [inst & rest-insts]]
 (if inst
 state
 (recur (act state inst) rest-insts)))

There's probably a way to use a recursion abstraction to avoid the explicit recursion here, but I can't think of one that would lead to neater code.

---

Really, the whole Vector record is unnecessary. By just using an actual Clojure vector instead, you can make use of all the standard functions instead of needing to write your own.

Want to add 2 to each element of your vector coordinate?

(mapv #(+ % 2) [1 2]) ;[3 4]

Want to add two coordinates together?

(mapv + [1 2] [3 4]) ;[4 6]

Easy. If you were using your Vector though, you would need to manually write such functions.

---

moveForward could also greatly benefit from destructuring. As soon as you begin manually using keyword accessors to get all the elements, consider destructuring:

(defn moveForward [{:keys [x y]} orientation distance]
 (let [{xv :x, yv :y} (orientation orientations)]
 (->Vector
 (+ x (* distance xv))
 (+ y (* distance yv)))))

This is even neater though if you just use Clojure vectors:

(defn moveForward [[x y] orientation distance]
 (let [[xv yv] (orientation orientations)]
 [(+ x (* distance xv))
 (+ y (* distance yv))]))

---

(require '(java.lang.Math))

is unnecessary. Math is always in scope already, as with all of java.lang.

---

*

I actually prefer to do destructuring in lets instead of the parameter vector. I find destructuring in parameters to look bloated.

I used parameter destructuring here for brevity, and because there seems to be a general trend to using it.

Answer 2

Take a look at Bruce Hauman's answer to the question: https://github.com/bhauman/advent-of-clojure-2016/blob/master/src/advent_of_clojure_2016/day1.clj

One thing that was an eye-opener to me is how we all (my answer had :east and :west etc) fall into the trap of having of describing reality rather than just solving the problem with as simple as possible data structures. You can see how little code he had to write - no need for State, no need for functions describing which way was turned. Albeit his advantage was knowing about reductions, which give a path back for every step taken.

• Simon Brahan: Thanks for the pointer. This code reads a little densely for my tastes, but there's a lot in there I didn't know about.