On Mon, Jan 17, 2011 at 1:47 AM, Stefan Rohlfing
<stefan.rohlf...@gmail.com> wrote:
> Hi all,
>
> I am trying to implement the function 'group-by' from Clojure.Core
> using my current knowledge of Clojure but cannot get pass a
> java.lang.Exception.
>
> This is the code so far:
>
> (defn key? [key coll]
> (some #{key} (keys coll)))
>
> (defn my-group-by [f coll]
> (let [test (fn [m x]
> (let [res (f x)]
> (if (key? res m)
> (conj (m res) x)
> (assoc m res (vec x)))))]
> (reduce test {} coll)))
First of all, your key? is only ever (supposed to be) called on maps,
so you can dispense with it:
(defn my-group-by [f coll]
(let [test (fn [m x]
(let [res (f x)]
(if (contains? res m)
(conj (m res) x)
(assoc m res (vec x)))))]
(reduce test {} coll)))
Second, your already-in-the-map case returns a vector. It doesn't
modify the vector in place; it makes a new one with one more item and
returns that. Not the map it should return.
The next iteration of your reduce, the vector gets passed to key? with
some value. Your key? function calls keys on it, which results in an
exception. I get:
user=> (keys ['x 'y 'z])
java.lang.ClassCastException: clojure.lang.Symbol cannot be cast to
java.util.Map$Entry
You got:
> ;; java.lang.Exception: Unable to convert: class java.lang.Integer to
> Object[] (repl-1:2)
I'm not sure why the difference -- different Clojure versions? -- but
no matter, the fix is the same:
(defn my-group-by [f coll]
(let [test (fn [m x]
(let [res (f x)]
(if (contains? res m)
(assoc m res (conj (m res) x))
(assoc m res (vec x)))))]
(reduce test {} coll)))
This actually returns an updated map and not just an updated vector.
But it can be made nicer in a couple of ways. One uses the optional
not-found option to get to dispense with the contains?:
(defn my-group-by [f coll]
(let [test (fn [m x]
(let [res (f x)]
(assoc m res (conj (m res []) x))))]
(reduce test {} coll)))
And the other uses update-in. Update-in means the key only needs to be
specified once, which lets you dispense with the inner let:
(defn my-group-by [f coll]
(let [test (fn [m x]
(update-in m [(f x)] #(if % (conj % x) [x])))]
(reduce test {} coll)))
This can be formatted a bit more nicely:
(defn my-group-by [f coll]
(reduce
(fn [m x]
(update-in m [(f x)]
#(if % (conj % x) [x])))
{} coll))
This gets rid of the outer let as well.
The latter two versions can't distinguish the map having a key with an
associated value of nil and the map lacking that key; however, the map
never associates a key with anything other than a vector, so this ends
up not mattering.
For reference (and I only peeked at this *after* writing all of the
above), the implementation of group-by in clojure.core is:
(defn group-by
"Returns a map of the elements of coll keyed by the result of
f on each element. The value at each key will be a vector of the
corresponding elements, in the order they appeared in coll."
{:added "1.2"
:static true}
[f coll]
(persistent!
(reduce
(fn [ret x]
(let [k (f x)]
(assoc! ret k (conj (get ret k []) x))))
(transient {}) coll)))
This is basically the same as the version I got adding the optional
not-found argument to get instead of using update-in, except that it
uses transients for the performance benefits (and has docstrings and
metadata) and gets rid of the outer let in the same manner as the
final update-in using version I wrote.
--
You received this message because you are subscribed to the Google
Groups "Clojure" group.
To post to this group, send email to clojure@googlegroups.com
Note that posts from new members are moderated - please be patient with your
first post.
To unsubscribe from this group, send email to
clojure+unsubscr...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/clojure?hl=en
