Clojure loop with cond

Question

I am working on a Project Euler problem that involves factoring numbers and have written the following function to do that.

(defn get-factors [value]
  (let [max-factor (->> (Math/sqrt value)
                        (Math/floor)
                        (Math/round))
        ifactors #{}]
   (loop [n 2 factors ifactors]
     (do 
       (println (format ">> N: %d, Factors: %s" n factors))
       (cond
        (when (> n max-factor)   ; exit of we have passed the max-factor
          (do 
            (println (format "--Exiting(%d): %s" n factors))
            factors))               ; return factors
        (when (= 0 (mod value n)); have we found a factor?   
          (do 
            (println (format"--Factor(%d)" n))
            (recur (inc n) (conj factors n (/ value n)))))   ; recurse: add _n_ and reciprocal _n_ to list
        :default (do             ; otherwise
                   (println (format"--default(%d): %s" n (= 0 (mod value n))))
                   (recur (inc n) factors)) ; recurse: increment _n_, dont modify factors
        )))))

However, the function is returning nil and my println statements are evaluated in a strange order. Here is the output from the REPL for (get-factors 12), which should return #{2,3,4,6}:

>> N: 2, Factors: #{}
--default(2): true
>> N: 3, Factors: #{}
--default(3): true
>> N: 4, Factors: #{}
--Exiting(4): #{}
--Factor(4)
>> N: 5, Factors: #{3 4}
--Exiting(5): #{3 4}

As you can see, the default state is being hit even though the (= 0 (mod value n)) of the previous case evaluates to true. Likewise, the exit condition is hit twice. The last case evaluated should be for n=3, but you can see output for up to n=5.

I'm obviously doing something fundamentally wrong but I am not seeing what. (Related, is there a better way to go about constructing a list?)

Answer 1

First, there is an implicit "when" (or "if") in the test part of any cond so you should not be using when yourself inside that test.

Second, you are using a single form, a when form, as the entire branch of the cond, therefore, the cond does not see the second form it expects when the test is true.

Check out this example of a proper cond:

http://clojuredocs.org/clojure_core/1.2.0/clojure.core/cond

Answer 2

If, as others have suggested, you strip out all the crud - the whens and dos and printlns - your program works!

A few suggestions:

• Use quot instead of / to get an integer quotient.
• You might as well start your threading macro thus: (->> value (Math/sqrt) ... ).
• Use :else, not :default or anything else to introduce the catch-all clause in your cond.