4 ms·

For me, this refactoring doesn't solve anything of note, and it introduces risks of its own. The function make-markov-mapping returns a weird intermediate data

by thom 11y ago

For me, this refactoring doesn't solve anything of note, and it introduces risks of its own. The function make-markov-mapping returns a weird intermediate data structure (a lazy sequence of tiny maps). You don't want that getting out into your codebase, which wasn't a risk in the original. We've also further embedded the string handling, which means when it comes to refactor away from just strings, we have more work to do and more places to do it. Also, just in terms of readability which is of course slightly better, we've still got things like nested threading macros which are a big cognitive overhead. Ultimately, we're revealing no new insight about the underlying problem and offering no new capabilities for the future.

thom 11y ago
So, worried that I was being overly-negative, I thought I'd offer something of substance. If you wanted to separate the Markov bits from the string bits, you could do it thusly: (defn markov-chain [coll] (let [adjacencies (partition 2 1 coll) initial-chain {::start [(first coll)] (last coll) [::end]}] (reduce (fn [chain [predecessor successor]] (assoc chain predecessor (conj (get chain predecessor []) successor))) initial-chain adjacencies))) (defn successor [chain item] (rand-nth (get chain item))) (defn follow-chain [chain] (->> ::start (iterate (partial successor chain)) (remove #{::start ::end}) (take-while some?))) (defn sentences [text] (clojure.string/split text #"\.")) (defn words [sentence] (remove empty? (clojure.string/split sentence #"\s+"))) (defn markov-text [text] (->> text sentences (map words) (map markov-chain) (apply merge-with into))) (defn render-sentence [words] (str (clojure.string/join " " words) \.)) (defn sentence [chain] (render-sentence (follow-chain chain))) I'm not saying this is in any way strictly better, but it leaves you with abstractions that can be reused, and text-specific stuff in one place that could be a separate namespace. This is what I'd look for in a code-review of a refactoring like this - not just neatness, but making abstractions explicit and separate, naming as many things as possible, etc etc.
o_nate 11y ago
I think the point of the refactoring was not to "solve" anything, and certainly not to make anything more abstract. Rather it was to make the code more readable. For me, a Clojure newbie, it succeeded admirably at that purpose.

by thom 11y ago

thom 11y ago

So, worried that I was being overly-negative, I thought I'd offer something of substance. If you wanted to separate the Markov bits from the string bits, you could do it thusly: (defn markov-chain [coll] (let [adjacencies (partition 2 1 coll) initial-chain {::start [(first coll)] (last coll) [::end]}] (reduce (fn [chain [predecessor successor]] (assoc chain predecessor (conj (get chain predecessor []) successor))) initial-chain adjacencies))) (defn successor [chain item] (rand-nth (get chain item))) (defn follow-chain [chain] (->> ::start (iterate (partial successor chain)) (remove #{::start ::end}) (take-while some?))) (defn sentences [text] (clojure.string/split text #"\.")) (defn words [sentence] (remove empty? (clojure.string/split sentence #"\s+"))) (defn markov-text [text] (->> text sentences (map words) (map markov-chain) (apply merge-with into))) (defn render-sentence [words] (str (clojure.string/join " " words) \.)) (defn sentence [chain] (render-sentence (follow-chain chain))) I'm not saying this is in any way strictly better, but it leaves you with abstractions that can be reused, and text-specific stuff in one place that could be a separate namespace. This is what I'd look for in a code-review of a refactoring like this - not just neatness, but making abstractions explicit and separate, naming as many things as possible, etc etc.

o_nate 11y ago

I think the point of the refactoring was not to "solve" anything, and certainly not to make anything more abstract. Rather it was to make the code more readable. For me, a Clojure newbie, it succeeded admirably at that purpose.