CODEWITHSUNDEEP
functionclojurerefactoringidioms
mzdravkov
mzdravkov

Reputation: 145

More idiomatic and elegant way of Clojure function

I have a function which finds the least distance between nodes in graph, written in Ruby. I translated it to Clojure, but in my opinion it looks terrible.

The representation of data looks like this:

hash = {:v0 [:v1  :v2  :v3]
        :v1 [:v4  :v5  :v6] 
        :v2 [:v7  :v8  :v9]
        :v3 [:v10 :v11 :v12]
        :v4 [:v13 :v14 :v15]}

The Ruby function looks like this:

 def distance src, target, hash
    return 0 if src == target
    return nil if hash[src].nil?
    dist = 1

    if hash[src].include? target
        return dist
    else
        arr = hash[src].map {|x| distance x, target, hash}
    end
    arr = arr.delete_if {|x| x.nil?}

    return dist + arr.min if !arr.empty?
    return nil
end

And the Clojure function looks like this:

(use 'clojure.contrib.seq-utils)
(defn distance [src target h] 
  (if (= src target)
    0
    (if (nil? (h src))
      nil
      (if (includes? (h src) target)
        1
        (let [arr (filter #(not= % nil) (map #(distance % target h) (h src)))]
          (if (= (empty? arr) true)
            nil
            (+ 1 (apply min arr))))))))

Can you show me a more elegant and Clojure-like way of doing this. Those nested ifs are terrible.

Upvotes: 2

Views: 228

Answers (5)

ponzao
ponzao

Reputation: 20934

Without any algorithmic changes you can shorten your code quite a bit (comments inline):

(defn distance [src target h] 
  (if (= src target)
    0
    (when (h src) ; nil is "falsy" so no need to check for it.
                   ; when's else evaluates to nil
      (if (includes? (h src) target)
        1
        (let [arr (keep #(distance % target h) (h src))] ; keep is same as map but drops nils
          (when-not (empty? arr) ; Same as above. Also empty? returns true or false.
            (inc (apply min arr)))))))) ; inc is used to increment by one

This could be shortened even further:

(let [arr (keep #(distance % target h) (h src))]
  (when-not (empty? arr)
    (inc (apply min arr))))

to this:

(when-let [arr (seq (keep #(distance % target h) (h src)))]
  (inc (apply min arr)))

Because seq returns nil for empty collections.

And as already mentioned by others, using Contrib nowadays is not a good idea.

Upvotes: 1

cgrand
cgrand

Reputation: 7949

If you are feeling seq-y:

(defn distance [src target h] 
  (if (= src target)
    0
    (->> src h
      (keep #(distance % target h))
      (map inc)
      (reduce #(if %1 (min %1 %2) %2) nil))))

Upvotes: 1

DanLebrero
DanLebrero

Reputation: 8593

Another suggestion. Calculate all the routes and then calculate the min distance:

(defn routes
  ([src target m]
     (if (= src target)
       [[]]
       (seq (routes src target m []))))
  ([src target m so-far]
     (if-let [near (get m src)]
       (if (contains? near target)
         [(conj so-far target)]
         (mapcat #(routes % target m (conj so-far %)) near)))))

(defn min-distance [src target m]
  (if-let [all-routes (routes src target m)]
    (apply min (map count all-routes))))

Upvotes: 0

Jan
Jan

Reputation: 11726

Noticing that filter generates a lazy seq allows to simplify Arthur Ulfeldt's answer a bit without sacrificing performance by removing the if. Additionally, contains? can also be omitted, but in this case the increase of readability is disputable.

(defn distance [src target h]
  (let [arr (filter #(not= % nil) (map #(distance % target h) (h src)))]
    (cond (= src target)   0
          (nil? (h src))   nil
          ((h src) target) 1
          (empty? arr)     nil
          :else            (+ 1 (apply min arr)))))

Upvotes: 2

Arthur Ulfeldt
Arthur Ulfeldt

Reputation: 91534

If you use sets instead of vectors and use cond instead of nested ifs it looks, to me at least, a bit more Clojure-like:

(def h {:v0 #{:v1  :v2  :v3}
        :v1 #{:v4  :v5  :v6}
        :v2 #{:v7  :v8  :v9}
        :v3 #{:v10 :v11 :v12}
        :v4 #{:v13 :v14 :v15}})

(defn distance [src target h]
  (cond (= src target) 0
        (nil? (h src))  nil
        (contains? (h src) target)
         :default (let [arr (filter #(not= % nil) (map #(distance % target h) (h src)))]
                   (if (empty? arr)
                     nil
                     (inc (apply min arr))))))

It is also worth noting that clojure.contrib is quite obsolete now. Removing it allows this code to run on most any version of Clojure.

Upvotes: 3

Related Questions