Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've been playing with Clojure for the last few evenings, going through the well known 99 problems (I'm using a set adapted for Scala).

Problem 26 calls for a function that, given a set S and a no. of items K, returns all possible combinations of K items that can be taken from set S.

So here's my solution:


(defn combinations [k s]
  (cond
    (> k (count s)) nil        ;not enough items in sequence to form a valid combination
    (= k (count s)) [s]        ;only one combination available: all items 
    (= 1 k) (map vector s)     ;every item (on its own) is a valid combination
    :else (reduce concat (map-indexed 
            (fn [i x] (map #(cons x %) (combinations (dec k) (drop (inc i) s)))) 
            s))))

(combinations 3 ['a 'b 'c 'd 'f])

My basic solution is to take each item from the given sequence (map-indexed) and recurse to generate combinations of size K - 1 from the remaining sequence. The termination conditions are described above.

I'm still a complete Clojure newbie and would welcome comments on structure, efficiency, readability, resemblance to idiomatic Clojure, etc. Feel free to be brutal, but please remember I've been doing Clojure for only a few hours :)

I'm less interested in alternative mathematical methods for generating k-combinations, more interested in feedback on whether this is passable Clojure.

Cheers!

share|improve this question
1  
Check how it is done in math.combinatorics github.com/clojure/math.combinatorics/blob/master/src/main/… –  thattommyhall Feb 2 '13 at 1:38
add comment

1 Answer 1

up vote 3 down vote accepted

I'm currently reading "Joy of Clojure" so I'm (very) far from being "fluent" in Clojure but what I noticed is:

  • your solution is clever but quite complicated, you use "imperative" habits like indexed iteration
  • try to keep with simple abstractions like sequence first and rest and the solution will work with any Clojure collection - see example below
  • your solution use cond with three checks for k - consider using condp

Here's my code:

(defn subsets [n items]
(cond
    (= n 0) '(())
    (empty? items) '()
    :else (concat (map
                    #(cons (first items) %)
                    (subsets (dec n) (rest items)))
                  (subsets n (rest items)))))
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.