Fork me on GitHub
#clojure
<
2024-02-25
>
Dallas Surewood09:02:33

I could use help cleaning up a clojure function I was doing for advent of code. I find nested if checks with different calls to recur really hard to read. Is there a way to make this more readable without extracting parts of it to other functions? One special consideration is the array we loop through may dynamically change over time, so using for doesn't really work.

(defn process-intervals [seed-intervals]
  (loop [intervals seed-intervals
         min-number ##Inf]
    (if-let [[current-interval & remaining-intervals] (seq intervals)]
      (let [[start _ current-interval-rules] current-interval]
        (if (empty? current-interval-rules)
          (recur remaining-intervals (min min-number start))
          (let [result-of-interval-process
                (process-interval-through-rules-row current-interval)]
            (if result-of-interval-process
              (recur
               (concat result-of-interval-process remaining-intervals) min-number)
              (recur remaining-intervals min-number)))))
      min-number)))

p-himik09:02:43

Here's a rewrite, done without thinking about the algorithm at all so maybe there's room for improvement:

(defn process-intervals [seed-intervals]
  (loop [intervals seed-intervals
         min-number ##Inf]
    (if-let [[current-interval & remaining-intervals] (seq intervals)]
      (let [[start _ current-interval-rules] current-interval]
        (if (empty? current-interval-rules)
          (recur remaining-intervals (min min-number start))
          (let [intervals (concat (process-interval-through-rules-row current-interval)
                                  remaining-intervals)]
            (recur intervals min-number))))
      min-number)))

p-himik09:02:04

Thinking about the algorithm itself a bit more, this is how I'd write it:

(defn process-intervals [seed-intervals]
  (letfn [(has-rules? [[_start _ rules]]
            (seq rules))]
    (->> (tree-seq has-rules?
                   process-interval-through-rules-row
                   seed-intervals)
         (remove has-rules?)
         (map first)
         (reduce min ##Inf))))

Omer ZAK09:02:19

What do you have against extracting parts to other functions?

Dallas Surewood09:02:14

Nothing, but you can't extract much due to the nested recur calls, so you would mostly be extracting tiny things that probably would make it less readable.

Dallas Surewood10:02:07

@U2FRKM4TW I'm checking out your second answer. It crashes for me because of what the functions are doing, but it's got fresh stuff I haven't seen. Maybe I just need to change one thing. Will get back to you. The first suggestions helped. I didn't realize (concat nil [1 2]) was the same as [1 2]

p-himik10:02:51

Hmm, curious why it crashes. It should be doing pretty much the same.

Dallas Surewood10:02:40

I think tree-seq might not be appropriate. The rules are an array of rows of rules. Like this

[[{:source-range-start 98, :source-range-end 100, :delta -48}
     {:source-range-start 50, :source-range-end 98, :delta 2}]
    [{:source-range-start 15, :source-range-end 52, :delta -15}
     {:source-range-start 52, :source-range-end 54, :delta -15}
     {:source-range-start 0, :source-range-end 15, :delta 39}]
    [{:source-range-start 53, :source-range-end 61, :delta -4}
     {:source-range-start 11, :source-range-end 53, :delta -11}
     {:source-range-start 0, :source-range-end 7, :delta 42}
     {:source-range-start 7, :source-range-end 11, :delta 50}]
    [{:source-range-start 18, :source-range-end 25, :delta 70}
     {:source-range-start 25, :source-range-end 95, :delta -7}]
    [{:source-range-start 77, :source-range-end 100, :delta -32}
     {:source-range-start 45, :source-range-end 64, :delta 36}
     {:source-range-start 64, :source-range-end 77, :delta 4}]
    [{:source-range-start 69, :source-range-end 70, :delta -69}
     {:source-range-start 0, :source-range-end 69, :delta 1}]
    [{:source-range-start 56, :source-range-end 93, :delta 4}
     {:source-range-start 93, :source-range-end 97, :delta -37}]]

Dallas Surewood10:02:16

This is my first time seeing it though, I'm playing with it

p-himik10:02:33

What is the exception?

Dallas Surewood10:02:58

I was wrong, I think this is working

p-himik10:02:27

BTW if the data is always a vector of vectors of maps, then there might be other solutions. Your initial solution and the tree-seq solution both can work on an arbitrarily nested data, so I made no assumptions.

p-himik10:02:48

Ah, those are only the rules, not the intervals, alright.

Dallas Surewood10:02:24

Something like this

(defn process-intervals [seed-intervals]
  (letfn [(has-rules? [[_start _ rules]]
            (seq rules))]
    (reduce min ##Inf
            (for [interval seed-intervals]
              (->> (tree-seq has-rules?
                             process-interval-through-rules-row
                             interval)
                   (remove has-rules?)
                   (map first)
                   (reduce min ##Inf))))))

Dallas Surewood10:02:39

I am having a hard time grokking tree-seq but that works

p-himik10:02:09

Ahh, of course, my bad. A slightly rearranged alternative:

(defn process-intervals [seed-intervals]
  (letfn [(has-rules? [[_start _ rules]]
            (seq rules))]
    (->> seed-intervals
         (mapcat (fn [interval]
                   (->> (tree-seq has-rules?
                                  process-interval-through-rules-row
                                  interval)
                        (remove has-rules?)
                        (map first))))
         (reduce min ##Inf))))

Dallas Surewood10:02:39

That works too. Playing around with tree-seq some more and trying to wrap around how that maps to what I was doing

p-himik10:02:57

Your intervals are nested, so they form a tree. tree-seq returns the nodes of that tree as a seq.

Dallas Surewood10:02:49

Hmm. The intervals are nested as in once the rules are applied to them? Because the array of rules doesn't seem nested

Dallas Surewood11:02:05

So it looks like if you want to apply a transformation to something, and it causes more of that thing in similar shapes that need to be processed the same way, tree-seq is good for that.

Dallas Surewood11:02:21

And the same function that determines when to stop processing can be used to remove the intermediate results.

Dallas Surewood18:02:16

I wrote some notes on tree-seq this morning for myself. Thank you for introducing me to it.

👍 1
Dallas Surewood19:02:55

Let me know if you have any ideas for this one too

(defn process-interval-through-rule [interval processing-rule]
  ;; Will take an interval and either return nil if there's no overlap
  ;; with the rule, or it will return an array of new intervals. 
  ;; The new intervals will be where the ranges don't overlap, and
  ;; also where the interval does overlap with one row of rules GONE
  ;; This is similar to advancing the level of the rule. 
  ;;
  ;; It doesn't care what is in interval-rules, it will merely remove the first
  ;; thing. It relies on processing-rule for the actual check. 
  (let [[interval-start interval-end interval-rules] interval
        {:keys [source-range-start source-range-end delta]} processing-rule]
    (if (or (<= interval-end source-range-start)
            (<= source-range-end interval-start))
      nil
      (let [within-start (< interval-start source-range-start)
            within-end (< source-range-end interval-end)]
        (keep identity
              [(when within-start
                 [interval-start source-range-start interval-rules])
               (when within-end
                 [source-range-end interval-end interval-rules])
               [(+ (if within-start source-range-start interval-start) delta)
                (+ (if within-end source-range-end interval-end) delta)
                (rest interval-rules)]])))))

p-himik19:02:27

Nothing groundbreaking, just some cosmetic stuff because I have different preferences (no single-branch ifs, ? at the end of most boolean variables, (remove nil? ...) instead of (keep identity ...)).

p-himik19:02:05

If the order of the resulting array doesn't matter, I'd use cond-> with conj to add items to a vector rather than remove them.

Dallas Surewood21:02:32

Do I have a single branch if?

p-himik21:02:13

I treat if + nil as a single-branch if most of the time.

p-himik09:02:30

Oh, almost forgot one tiny thing - you're using the word "array" where a correct term should be "vector". Clojure has both (arrays are just Java arrays), they're different.

Filipe Silva11:02:40

I'm trying to use the new clojure.repl.deps/add-lib described in the https://clojure.org/news/2023/04/14/clojure-1-12-alpha2#_add_libraries_for_interactive_use, but as far as I can tell it requires 1.12 installed globally. Is there an ergonomic way of installing the 1.12 alpha globally? I tried fiddling around with the global deps.edn but that didn't seem to work. https://github.com/clojure/homebrew-tools talks about having pre-release versions, but it doesn't seem to have the 1.12 alpha.

p-himik12:02:15

> it requires 1.12 installed globally No. You don't install Clojure globally, you install only the CLI globally. And that feature requires the latest version of the CLI, not of Clojure itself.

p-himik12:02:22

E.g. the feature works just fine with 1.11.1.1435. But you have to run clj with -Sdeps '{:deps {org.clojure/clojure {:mvn/version "1.12.0-alpha8"}}}' in order for it to work.

Filipe Silva12:02:04

uhm when I tried with alpha 2 locally, together with latest stable cli I got an error that I thought I traced down to missing functionality in the global cli.... but maybe I messed up along the way

Filipe Silva12:02:08

gonna retry it right now

Filipe Silva12:02:52

yeap you're absolutely right

Filipe Silva12:02:13

[I] filipesilva@Filipes-MacBook-Pro ~/s/scratch (master) [SIGINT]> clj -Sdeps '{:deps {org.clojure/clojure {:mvn/version "1.12.0-alpha8"}}}'
Clojure 1.12.0-alpha8
user=> (require '[clojure.repl.deps :as deps])
nil
user=> (deps/add-lib 'org.clojure/data.csv {:mvn/version "1.1.0"})
[org.clojure/data.csv]
user=>

Filipe Silva12:02:33

dunno what I did before that was broken, thanks for setting me on the right track!

👍 1