CODEWITHSUNDEEP
clojurefunctional-programmingpurely-functional
Post Self
Post Self

Reputation: 1554

Making Clojure let statement more functional

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.

Upvotes: 2

Views: 194

Answers (3)

kopos
kopos

Reputation: 405

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

Your code is already pretty neat and well laid out. The only change I would want to do, is stay in the domain as long as possible - in this case the signs objects. And use the counts only much later.

The return value can then use print to do the actual actions.

Upvotes: 1

Taylor Wood
Taylor Wood

Reputation: 16194

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

Upvotes: 9

Micah Elliott
Micah Elliott

Reputation: 10264

Your code doesn't have side-effects (except for printing, which we'll fix), so I wouldn't say it's not functional. let is designed for building up intermediate values. In comments are some potential improvements.

                      ; align here (controversial)
(let [broken-signs    (->> (:symbols field)
                           (map make-sign)
                           (filter broken?))
      broken-count    (count broken-signs)
      unfixable-count (->> broken-signs ; maybe emphasize non-function start
                           (filter (complement fixable?))
                           count)] ; no parens needed
  ;; don't print; just return number
  (if (< 0 unfixable-count)  ; prefer less-than (Elements of Clojure book)
    -1
    (- broken-count unfixable-count)))

I highly recommend the Clojure Style Guide for related suggestions.

Upvotes: 0

Related Questions