Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have this set of functions - it works, but I'm concerned about the use of lets.

(defn- create-counts [coll]
  "Computes how many times did each 'next state' come from a 'previous state'.
   The result type is {previous_state {next_state count}}."
  (let [past    coll
        present (rest coll)
        zipped  (map vector past present)
        sorted  (sort zipped)
        grouped (group-by first sorted)
        seconds (map (fn [[key pairs]] [key (map second pairs)]) (seq grouped))
        freqs   (map (fn [[key secs]]  [key (frequencies secs)]) seconds)]
    (into {} freqs)))

I started learning functional programming in Haskell some time ago, and there, because of laziness, it's commonplace to use this equivalent of Clojure let, because what's not needed, isn't ever computed:

asnwer = z
  where
    notused = map (^1000) [1000..10000]
    x = [1..]
    y = take 10 x
    z = map (*2) y
  • Is it okay to use Clojure let in this way? Look for example at the create-counts fn definition - I use it quite heavily there.
  • Would it be better to just put it all inside one expression?
  • What's the best practice for this?

I like how everything's named, but I don't know if there's not a performance penalty hidden somewhere.

When I try to match the Haskell example with Clojure code, it seems there's eager evaluation:

(let [notused (do (Thread/sleep (rand 10000000)) :done)
      x       (iterate inc 1)
      y       (take 10 x)
      z       (map #(* 2 %) y)]
  z)

;=> sleeps till the judgement day 

I could imagine preventing this with futures or some clever let-like macro, but really it seems to me that I'm trying to do something Haskell-y (lazy) in Clojure. What's the idiomatic way?

share|improve this question
    
Just a note that the Haskell equivalent of the clojure let is let. where is a different beast –  jozefg Feb 25 '13 at 0:36
    
Thanks, jozefg. Links with more info: haskell.org/tutorial/patterns.html#sect4.5 and haskell.org/haskellwiki/Let_vs._Where –  Martin Janiczek Feb 25 '13 at 9:44
    
I'm a clojure noob, but the documentation for map says it returns a lazy sequence. Group-by is not lazy. clojuredocs.org/clojure_core/clojure.core/map –  GlenPeterson Feb 25 '13 at 13:57

1 Answer 1

up vote 4 down vote accepted

First of all, you could use abstract structural binding (a.k.a. destructuring) to reduce the number of let bindings:

(defn- create-counts_org [[_ & present :as coll]]
  "Computes how many times did each 'next state' come from a 'previous state'.
   The result type is {previous_state {next_state count}}."
  (let [zipped  (map vector coll present)
        sorted  (sort zipped)
        grouped (group-by first sorted)
        seconds (map (fn [[key pairs]] [key (map second pairs)]) (seq grouped))
        freqs   (map (fn [[key secs]]  [key (frequencies secs)]) seconds)]
    (into {} freqs)))

I think there's nothing wrong in using let bindings that are technically not necessary to improve readability and comprehensibility.

But in your function, I think it is pretty clear what every step does:

  • sorted -> the first thing I see is the sort function
  • grouped -> the first thing I see is the group-by function
  • seconds / freqs -> I think these are also pretty clear, using second and frequencies

And because each let binding just uses the previous binding once, I would probably use the thread-last (->>) macro:

(defn- create-counts [[_ & rest :as coll]]
  "Computes how many times did each 'next state' come from a 'previous state'.
   The result type is {previous_state {next_state count}}."
  (->>  (map vector coll rest)
        (sort)
        (group-by first)
        (map (fn [[key pairs]] [key (map second pairs)]))
        (map (fn [[key secs]]  [key (frequencies secs)]))
        (into {})))

I think this also answers your better to just put it all inside one expression question. Also, even if there where a performance penalty in using let bindings, it would probably be negligible.

share|improve this answer
    
Thanks! I've seen the ->> macro before, but didn't think about it. Seems handy. –  Martin Janiczek Feb 28 '13 at 9:48

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.