Fork me on GitHub
#code-reviews
<
2020-07-01
>
adam20:07:19

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

noisesmith20:07:20

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

1
noisesmith20:07:53

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

noisesmith20:07:17

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

adam20:07:39

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

noisesmith20:07:55

no, that's a perfect usage of let

noisesmith20:07:38

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

noisesmith20:07:54

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

noisesmith20:07:54

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

adam20:07:18

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

noisesmith20:07:13

right - my concern is whether it would be correct

noisesmith20:07:35

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

noisesmith20:07:19

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

phronmophobic20:07:50

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

noisesmith20:07:30

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

walterl20:07:08

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

noisesmith20:07:52

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

noisesmith20:07:32

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

walterl20:07:29

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.

noisesmith21:07:36

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

noisesmith21:07:19

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

noisesmith21:07:04

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

walterl21:07:07

> rule of thumb

noisesmith21:07:45

fair, I missed that

walterl21:07:21

There are many exceptions to that rule.

walterl21:07:36

Trackable steps being a good one

practicalli-john10:07:43

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.

💯 1
noisesmith14:07:27

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

practicalli-john15:07:47

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

adam20:07:19

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

adam21:07:24

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