Avoiding duplicated code in flow control

[miedwar]

Hi All,

I’m starting out with Clojure and I’m wondering what’s the idiomatic way to write the following code snippet?

(if-let [last-time (get-last-time uid)]
  (when (> current-time last-time)
    (do-something-with-side-effect uid)
    (update-last-time! current-time))
  (update-last-time! current-time)))

If I don’t have a ‘last time’, update the cache with current-time, if I do, but last time is newer, do something and also update the cache.

I’m bothered by the repetition of the update cache function.

Thanks!

[p-himik]

Personally, I would be perfectly fine with the above code. Not everything needs to be deduped.

But there are alternatives. You can add something like update-last-time-if-newer! or you can put true there instead of the duplicated line and wrap it all in (when ... (update-last-time! current-time)) (assuming you don’t use the result of that if-let currently).

[joinr]

maybe…

(if (some->> (get-last-time uid)
             (> current-time))
  (do-something-with-side-effect uid)
  (update-last-time! current-time))

Minor code golf though. I like some-> and some->> for threading while nil punning; fairly useful for composing nil-able things.

[Harleqin]

If you want to update-last-time! in any case, then you should do it outside of the conditional form:

(when (some-> (get-last-time uid) (< current-time))
  (do-something-with-side-effect uid))
(update-last-time! current-time)

I’d like to add, since you mention a cache, it is often necessary to have some transactional behaviour for that, e. g. using an atom and swap! or swap-vals!.

[DrLjotsson]

I think that the duplication is fine. But the code is a bit hard to read because if the when statement is false, (update-last-time! current-time) is never executed. This intent is not clear in the code. I would suggest something more explicit.

(let [last-time (get-last-time uid)]
  (cond
    (and last-time (> current-time last-time))
    (do (do-something-with-side-effect uid)
        (update-last-time! current-time))

    (nil? last-time)
    (update-last-time! current-time)))

[ryantate]

I think @DrLjotsson is correct that the intent is not clear, obscuring the code.

I would go a step further and decomplect the do-something action from the update-last-time! action. This makes the intent even clearer, in particular it makes it clearer under what conditions update-last-time! will execute, because you only need to look at a single condition rather than reasoning about how two conditions interact:

(let [last-time (get-last-time uid)]
  (when (and last-time (> current-time last-time))
    (do-something-with-side-effect uid))
  (when-not (and last-time (<= current-time last-time))
    (update-last-time! current-time)))