Clojure - sum up a bunch of numbers

Question

Hey I'm doing a Project Euler question, and I'm looking to sum up all the numbers under 1000 that are multiplies of 3 or 5.

But being a clojure noob, my code just keeps returning zero.. and I'm not sure why.

(defn sum-of-multiples [max]
  (let [result (atom 0)]
    (for [i (range max)] 
      (if (or (= (rem i 3) 0) (= (rem i 5) 0))
        (swap! result (+ @result i)))    
      )
    @result))

(sum-of-multiples 1000)

Also the line (swap! result (+ @result i))) bugs me.. In C# I could do result += i, but I'm guessing there must be a better way to this in Clojure?

Answer 1

In Clojure - and at large in functional programming - we avoid assignment as it destroys state history and makes writing concurrent programs a whole lot harder. In fact, Clojure doesn't even support assignment. An atom is a reference type that is thread safe.

Another common trait of functional programming is that we try to solve problems as a series of data transformations. In your case you case some data, a list of numbers from 0 to 1000 exclusive, and you need to obtain the sum of all numbers that match a predicate. This can certainly be done by applying data transformations and completely removing the need for assignment. One such implementation is this:

(->> (range 1000)
     (filter #(or (= (rem % 3) 0) (= (rem % 5) 0)))
     (reduce +))

Please understand that a function such as the one you wrote isn't considered idiomatic code. Having said that, in the interest of learning, it can be made to work like so:

(defn sum-of-multiples [max]
  (let [result (atom 0)]
    (doseq [i (range max)] 
      (if (or (= (rem i 3) 0) (= (rem i 5) 0))
        (swap! result #(+ % i)))    
      )
    @result))

(sum-of-multiples 1000)

for returns a lazy sequence but since you're simply interested in the side-effects caused by swap! you need to use doseq to force the sequence. The other problem is that the second argument to swap! is a function, so you don't need to deref result again.

Answer 2

for is a list comprehension that return a lazy sequence, you have to traverse it for your code to work:

(defn sum-of-multiples [max]
  (let [result (atom 0)]
    (dorun
      (for [i (range max)] 
        (if (or (= (rem i 3) 0) (= (rem i 5) 0))
          (swap! result + i))))
    @result))

---

An equivalent, more idiomatic implementation using for:

(defn sum-of-multiples [max]
  (reduce +
    (for [i (range max)
          :when (or (zero? (rem i 3))
                    (zero? (rem i 5)))] 
      i)))

Answer 3

The other answers are good examples of what I alluded to in my comment. For the sake of completeness, here's a solution that uses loop/recur, so it may be easier to understand for someone who's still not comfortable with concepts like filter, map or reduce. It also happens to be about 30-40% faster, not that it really matters in this case.

(defn sum-of-multiples [max]
  (loop [i 0
         sum 0]
    (if (> max i)
       (recur (inc i)
              (if (or (zero? (rem i 3)) (zero? (rem i 5)))
                (+ sum  i)
                sum))
        sum)))