Fork me on GitHub

Which one of these two functions is considered better:

(defn navbar-toggle []
  (when-let [burger-elem (gdom/getElementByClass "navbar-burger")]
    (let [target-name (gdataset/get burger-elem "target")
          target-elem (gdom/getElement target-name)]
        (fn [e]
          (gclass/toggle burger-elem "is-active")
          (gclass/toggle target-elem "is-active"))))))
(defn navbar-toggle []
  (when-let [burger-elem (gdom/getElementByClass "navbar-burger")]
    (gevents/listen burger-elem EventType.CLICK (fn [_]
                                                 (gclass/toggle burger-elem "is-active")
                                                 (gclass/toggle (gdom/getElement (gdataset/get burger-elem "target")) "is-active")))))


I like the first, though I'd use _ instead of e since e isn't used


the second one is character and line count but in a way that makes it harder to read


their performance will be identical at runtime if I am not misreading


Ok thanks, I needed a confirmation that I am not abusing let 🙂


no, that's a perfect usage of let


one thing though - how often would gdataset/get return a different result if called later in the callback?


if that answer isn't "never", you should move the let into the fn


that goes for gdom/getElement too of course - to be safe I'd probably want to move the let into the fn, and view hoisting it out as a potential optimization


Wouldn’t that be theoretically slower? I assume the binding will be recreated on every click on the element


right - my concern is whether it would be correct


for example, with react you can see the dom nodes destroyed entirely and replaced


but maybe your knowledge of your app lets you know that the element remains valid in all contexts where that click can happen - that's why I referred to it as an optimization


even if it's theoretically slower, it's unlikely to make a practical difference unless (gdataset/get burger-elem "target") is really slow


right, "root of all evil" and all that, only do optimizations when you know they will matter because they carry a cost


A colleague follows the rule of thumb that let-bindings that are only used once, are superfluous, as it is in the first implementation. I've come around to defaulting to that too. The second could be made a bit easier to read, though:

(defn navbar-toggle []
  (when-let [burger-elem (gdom/getElementByClass "navbar-burger")]
    (gevents/listen burger-elem
                    (fn [_]
                      (gclass/toggle burger-elem "is-active")
                      (-> (gdataset/get burger-elem "target")
                          (gclass/toggle "is-active"))))))


> let-bindings that are only used once, are superfluous


hard disagree, a let binding is free in terms of performance / implementation complexity, and increases clarity


Isn't "hard" a bit of a... hard position to take? 😝 let-bindings add to the cognative load of the reader, and, names being hard, will often not increase clarity. There's also something to be said for allowing the lispy structure of the (helpfully indented) code to aid in its visual parsing. A part of my colleague's argument that also made sense to me is that, if the inline invocation isn't easy/easier to understand, the problem may lie with the interface(s) of the called function(s). As a concrete example, something like (let [full-name (get-full-name first-name last-name)] ...) would be worse than using (full-name first-name last-name) in the body, in most cases. I think @UAX4V32SZ's function is also a great (and more practical) example for this. I'd rather go for the let-free version, for readability's sake.


in my experience, code where I replaced let with inline forms has been harder to maintain, let bindings don't add to cognitive load because I know precisely where to look for them and the formatting (if done idiomatically) makes it immediately clear where they are


note that I wasn't making a sweeping declaration that let bindings always increase clarity, I was objecting to a sweeping declaration that let bindings that are only used once are superfluous


let bindings that are used once are especially useful to break the job of reading carefully into distinct and trackable steps


> rule of thumb


fair, I missed that


There are many exceptions to that rule.


Trackable steps being a good one


In my opinion, inverting the rule of thumb would be more useful. "If something is used more than once, consider a let binding" Defining practices in the negative are less likely to be adhered to, as they block rather than guide. I also find let bindings offer more clarity to code, which is vital if there is a production issue that needs to be fixed quickly.

💯 3

I like that way of formalating it, and will be adopting the "guide rather than block" criterion 😄 thanks


consider it a small contribution compared to everything I have learnt from you here.. thank you 🙂


I tried to use this version of when-let to group things, but still struggling with compiling macros for CLJS.

(defmacro when-let*
  "Multiple binding version of when-let"
  [bindings & body]
  (if (seq bindings)
    `(when-let [~(first bindings) ~(second bindings)]
       (when-let* ~(vec (drop 2 bindings)) [email protected]))
    `(do [email protected])))


Ended up with this:

(defn navbar-toggle []
  (when-let* [burger-elem (gdom/getElementByClass "navbar-burger")
                 target-name (gdataset/get burger-elem "target")
                 target-elem (gdom/getElement target-name)]
      (gevents/listen burger-elem EventType.CLICK (fn [_]
                                                    (gclass/toggle burger-elem "is-active")
                                                    (gclass/toggle target-elem "is-active")))))