Clojurians Log v2

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)]
      (gevents/listen
        burger-elem
        EventType.CLICK
        (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
                    EventType.CLICK
                    (fn [_]
                      (gclass/toggle burger-elem "is-active")
                      (-> (gdataset/get burger-elem "target")
                          gdom/getElement
                          (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

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)) ~@body))
    `(do ~@body)))

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 @somedude314'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

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")))))