6 ms·
Rules for Writing Clojure Programs
- praptak 16y agoA tradeoff to be aware of: the introduction of sort raises the complexity of the solution from O(n) to O(nlogn).
- aphyr 16y agoI saw that and flipped out. It also requires an extra two scans through the list, in addition to the memory overhead for the temporary structures. Happily, the merge-with suggestion is much more efficient.
- amalcon 16y agoThe enormous irony here is that this is one of the rare cases that you could actually speed up using a non-comparison sort, like a counting-sort... except that this defies the point, because the first half of counting-sort is solving the problem we originally set out to solve.
- shasta 16y agoI was going to complain that parts of the article were disjointed and contradictory (Use -->, don't use -->, JK use it) until I noticed the name of the blog is "Two Guys Arguing." Solution from the comments: (apply merge-with + (map (fn [c] {c 1}) “abcdaabccc”))
- runevault 16y agoIt always amazes me how many people either never noticed merge-with or simply don't use it. Having done something similar with a list of words (created via regex parsed over a file) I adore what you can do with it. Especially if you use transients. Unless of course they just didn't think of the fact strings are seqs in clojure by default. edit: I could be crazy, but one problem with using apply is you force the realization of your object, so while in THIS case it's fine, using larger amount of data would cause a problem. IIRC more space sane way to do this is use (reduce {} (merge-with + %) list) and man I MUST be rusty, I can't remember if the default value comes before or after the function, and no repl handy. Hopefully the point is gotten across :)
- runevault 16y agoReplying to myself. Just googled it and I had the code wrong shame (defn count-words [coll] (reduce #(merge-with + %1 {%2 1}) {} coll))
- jared314 16y agoLooking from the outside in, clojure has too many useful/amazing functions to learn in an afternoon, weekend, or a month. So, not using something you don't understand is perfectly valid in my opinion.
- ataggart 16y agoThe problem with the latter solution is that apply isn't lazy. The more correct solution would be a simple reduce: (reduce #(update-in %1 [%2] (fnil inc 0)) {} "abcdaabccc")
- runevault 16y agoI wonder if update-in or merge-with would be more efficient. Another thing to consider is if you want to get fancy (persistent! (reduce #(update-in %1 [%2] (fnil inc 0)) (transient {}) "abcdaabccc")) Still no REPL but this should work and take advantage of transients if you're doing it against a bigger dataset (which I assume you care about since you also worried about laziness)
- deleted 16y ago[deleted]
- praptak 16y agoApply itself is lazy. This: (apply f infinite-lazy-list) does not force evaluation of the infinite-lazy-list, if the f itself does not force it. My favorite example is (apply concat ...), which can be used to flatten an infinite sequence of sequences (concat is lazy wrt to its argument list, so this works.)
- d0m 16y agoDon't take it harsh, but if it takes all this for this simple theoretical problem, imagine for a real world problem. In my opinion, a pure iterative approach is really simpler: (pseudo-code) : <code> myset = {} foreach c in mystring: if myset[c]: myset[c] += 1 else: myset[c] = 1 myset.sort(fn x,y: myset[x] < myset[y]) </code> As I said, it's all about opinion (and I know there are million ways to simplify that or make it different, i.e. myset.get() or whatever), but I find the imperative approach way easier to gasp at first glance. Compare that to: (let [a (sort str) b (partition-by identity a) c (map (juxt first count) a)] (sort-by second > c)) First, I don't really like a,b,c as name variable (to be fair, myset and mystring are terrible too). Second, I find there are so many extra step to resolve this simple problem.. we need: partition-by, we need identity, we need juxt (wtf is juxt?). Third, wtf are first, count and second..? I know high-level function are kewl, and working with lists are even cooler and using all of this in a 4 lines in a pretty clean way is again even cool-cooler. However, when I read this, even if there are less lines than the imperative way, it still takes more time to understand what the code is doing.
- s1rech 16y agowell, you take more time because you are not used to it. It is a bit like Linq in C#, at first it looks very foreign but if you use it for a while you start replacing classical loops with this list based constructs. At the very least, it is a good way to get another perspective.
- sanderjd 16y agoI totally get this argument. I also think it's an old-dog-new-tricks problem more than anything else. Things like "foreach", "in", "myset[c]" are part of our existing vocabulary and things like "let", "partition-by", "identity", "map", "juxt" are not (and also, "wtf are first, count, and second..?" is pretty dramatic if you ask me - these are just what they sound like and not at all confusing). You have to learn a new vocabulary to be effective in these languages - maybe the size of the vocabulary is bigger, or the scope of each "word" is larger, but we deal with abstraction all the time; you have to trust that a method or a syntax feature does what the documentation says it does (or read the code for it yourself) regardless of the language.
- 16y ago
- Raphael_Amiard 16y agoMy naive 2 minutes solution : (reduce #(assoc %1 %2 (inc (get %1 %2 0))) {} "abcaaabcccaabrrcb") EDIT: While it's shorter and (in my opinion) a lot clearer than the OP's solution, it's also consistently faster. Functionnal code and composition is cool, but i think the author got trapped into making his code beautifull, or maybe just into the wrong algorithm for the job. It happened to me before in clojure and other functional languages
- Lozzer 16y agoOr as mentioned in the comments (frequencies "abcaaabcccaabrrcb") with some optional sorting if that's required.
- bitsai 16y agoA friend and I are working through Project Euler as part of our effort to learn Clojure. My code is littered with the pattern of (map #(list % (f %)) coll) all over the place... Very glad that I read this article (and the comments), and I think it's time for me to start exploring the clojure and clojure-contrib API's in earnest.
- herdrick 16y ago(frequencies "abcaaabcccaabrrcb")
- Avshalom 16y agoas a note, the source is: (defn frequencies "Returns a map from distinct items in coll to the number of times they appear." {:added "1.2"} [coll] (persistent! (reduce (fn [counts x] (assoc! counts x (inc (get counts x 0)))) (transient {}) coll))) which I think is pretty much just Raphael's solution.
- KirinDave 16y agoI and many other experienced people on #clojure@freenode looked at this. Heard comments such as: “Huh.” “I can't believe -> and ->> are being demonized now.” ”I think if you're calling things rules you should be somewhat careful about how you phrase them...” “Someone is wrong on the internet.”
- alnayyir 16y agoSounds about right. Since you're apparently familiar with both Clojure and Erlang, what are your feelings regarding the concurrency model in each?
- KirinDave 16y agoThey are both good for different things. :) The clojure model means state is easy to move around. This is awesome. The Erlang model makes the server-like nature of each component very explicit, which can significantly reduce the complexity of some types of systems.
- alnayyir 16y agoCool, thanks for your thoughts. I always felt like Clojure was attempting a permissive system, whereas Erlang really is pretty up-front about the environment it's designed to work in. I keep meaning to fiddle with Reia but never got around to it.
- sqrt17 16y agoThe real reason why you want ->> can be seen in the code example for rule #7: instead of mapping the result of partitioning, it (wrongly) reuses a (i.e., the original list). If bugs result in irregularity, then making regularity explicit (by using ->> or other macros/higher-order functions) can be a good thing.
- KirinDave 16y agoGood catch! That's a very good point, and a very hard bug to see when you "get lazy" and use a, b, c as step names.
- WilliamLP 16y agoCan someone convince me what is gained by stressing out over making an easy solution to a trivial problem concise and elegant like this? You have a perfectly good imperative solution which is the way almost any child would do it (go through things one by one and keep counts of every bucket), so what is gained by messing with that? Is it not time to move on to the rest of the program?
- derefr 16y agoYou're thinking like someone who most often writes code. Think like someone who most often reads code—a maintenance programmer, say—and the benefits become obvious. We get away in this industry with writing things that, upon being encountered again, immediately provoke "throw it away and start again" comments. This sort of code will not provoke those sentiments. (That's not to say that it's actually good code—the article itself points out the problems—but those problems can actually be fixed by modification, not replacement, of the code, which is what we should all be striving for.)
- gruseom 16y agoThis sort of code will not provoke those sentiments. But it has provoked those sentiments in nearly everyone who's commented. Including the very comment you're replying to! I don't see anything about the original example that makes it less prone to being rewritten. Nothing's going to stop a programmer who's hell-bent on rewriting something. And "modification" and "replacement" are not that different anyway, especially when the scale is small.
- WilliamLP 16y agoAre you really going to defend that this isn't trivial to read: str = "abcaaabcccaabrrcb" dict = {} for ch in str: if ch not in dict.keys(): dict[ch] = 0 dict[ch] = dict[ch] + 1 sortedItems = sorted(dict.items(), key=lambda x: x[1], reverse=True)
- koenigdavidmj 16y agoimport operator str = "abcaaabcccaabrrcb" dict = {ch: str.count(ch) for ch in str} sorted_items = sorted(dict.items(), key=operator.itemgetter(1), reverse=True) The dictionary comprehension syntax is new in Python 2.7 and 3k, but you can say dict((ch, str.count(ch)) for ch in str) instead.