Making Clojure let statement more functional

Question

In my first Clojure project everything turned out nicely except this part at the end:

(let [broken-signs    (->> (:symbols field)
                           (map make-sign)
                           (filter broken?))
      broken-count    (count broken-signs)
      unfixable-count (-> (filter (complement fixable?) broken-signs)
                          (count))]
  (println
    (if (> unfixable-count 0)
      -1
      (- broken-count unfixable-count))))

The indentation looks off and it does not feel functional, since I am reusing state in the let block. I basically count the number of broken signs and then the number of fixable signs. If any sign is unfixable, I print -1, otherwise I print the number of signs to be fixed.

If I mapped/filtered twice, I'd have duplicate code, but most of all it'd run slower. Is there a way of improving this code nevertheless?

EDIT: This is what I settled on

 (defn count-broken-yet-fixable []
   (let [broken (->> (:symbols field)
                     (map make-sign)
                     (filter broken?))
         unfixable (remove fixable? broken)]
     (when (empty? unfixable)
       (count broken))))

 (defn solve-task []
   (if-let [result (count-broken-yet-fixable)]
     result
     -1))

(println (solve-task))

The subtraction was indeed not necessary and the count did not have to happen in the let block. Outputting the -1 on bad input also isn't the job of the function and is only part of the task.

Answer 1

I don't think there's anything "non-functional" or wrong with your approach. The indentation looks fine.

(let [broken (->> (:symbols field)
                  (map make-sign)
                  (filter broken?))
      unfixable (remove fixable? broken)]
  (when (seq unfixable)
    (- (count broken) (count unfixable))))

• You could replace filter (complement with remove
• Could use pos? instead of (> n 0)
• I might put two printlns inside the if, but really it's better to return a value
• You could inline the broken-count binding, since it's only used in one place
• I personally think this is easier to read with less threading macros
• Since the need to count unfixables is conditional, you could test for values with seq first
• If you return -1 as a sentinel value, I'd use nil instead; this happens naturally when the when condition isn't met
• The conditional logic seems backwards: you return -1 when unfixable-count is positive, and only use its value when it's not positive (which means it's zero b/c count can't be negative), e.g. it could be rewritten as (- broken-count 0) and then just broken-count