4
\$\begingroup\$

I would like to generate a deck of cards:

{:card-id 1 :suit :spade :rank 1}
{:card-id 2 :suit :spade :rank 2}
...
{:card-id 52 :suit :club :rank 13}

And here is my attempt:

(defn new-deck []
  (let [cards (flatten
                 (for [suit [:spade :heart :diamond :club]]
                    (for [rank [1 2 3 4 5 6 7 8 9 10 11 12 13]]
                      {:suit suit :rank rank} )))]
     (map-indexed (fn [idx itm] {:card-id (inc idx)
                                 :suit    (:suit itm)
                                 :rank    (:rank itm)})
                  cards)))

The code works but I think it is awkward. Especially the part where I use map-indexed function to add :card-id to each card.

Any suggestions to improve the code snippet?

\$\endgroup\$
0

3 Answers 3

Reset to default
7
\$\begingroup\$

There are a few minor improvements you can make here that lead to a fairly elegant implementation.

First, using flatten is rarely a good idea, because rather than just dealing with the top-level structure of the thing you hand it, it reaches down into its inner structure, thus making it brittle when you change the representation of your data. Therefore, you should instead use apply concat in place of flatten in your code.

However, as @ChrisMurphy points out, the for macro already supports list products, so you can replace the usage of flatten or apply concat and the two usages of for with a single usage of for that produces a flat sequence.

Then, instead of writing out the full range of numbers from 1 to 13, you can use the range function to replace that vector with (range 1 (inc 13)) or something equivalent.

Finally, rather than manually deconstructing and reconstructing each map in your lambda that you pass to map-indexed, you can use the assoc function to simply add the :card-id mapping.

With these changes, you end up with a solution that looks like this:

(defn new-deck []
  (map-indexed
   #(assoc %2 :card-id (inc %1))
   (for [suit [:spade :heart :diamond :club]
         rank (range 1 (inc 13))]
     {:suit suit :rank rank})))
\$\endgroup\$
4
  • \$\begingroup\$ And an alternative to apply concat is mapcat identity. \$\endgroup\$ Commented Mar 15, 2017 at 0:48
  • 1
    \$\begingroup\$ @ChrisMurphy Yes, but I can't see any reason you would want to use mapcat identity over apply concat; it's less readable (at least to me), it's longer, and mapcat is implemented in terms of apply concat anyway, so it should be slower as well. \$\endgroup\$ Commented Mar 15, 2017 at 1:13
  • \$\begingroup\$ All true, yet still its a good thing to know. \$\endgroup\$ Commented Mar 15, 2017 at 2:17
  • \$\begingroup\$ @ChrisMurphy True; from a quick search on GitHub, it looks like apply concat is only about 6 times as common as mapcat identity, so it's probably still useful to know the latter when reading other people's code. \$\endgroup\$ Commented Mar 15, 2017 at 2:23
2
\$\begingroup\$

Generation suggests cartesian product which suggests for comprehension:

(def ranks [1 2 3 4 5 6 7 8 9 10 11 12 13])
(def suits [:spade :heart :diamond :club])

(defn x-1 []
  (let [counter (atom -1)]
    (for [rank ranks
          suit suits]
      {:rank rank :suit suit :card-id (swap! counter inc)})))

I'm leaving the (atom -1) as an alternative to @Sam Estep's (and your) superior use of map-indexed, which I didn't think of at the time of answering, and now can't really edit!

Edit For completeness, and as this is kind of a code review, here's another way of using map-indexed:

(defn gen-hands []
  (for [rank ranks
        suit suits]
    {:rank rank :suit suit}))

(defn x-1 []
  (->> (gen-hands)
       (map-indexed (fn [idx m]
                      (assoc m :card-id idx)))))
\$\endgroup\$
0
0
\$\begingroup\$

For a third take on this problem, you can use the generator/yield style of programming popular in Python. This solution makes use of the lazy-gen & yield combination from the Tupelo library:

(ns tst.clj.core
  (:use clojure.test tupelo.test)
  (:require  [tupelo.core :as t]  ))
(t/refer-tupelo)

(defn new-deck []
  (lazy-gen
    (let [id (atom 0)]
      (doseq [suit [:spade :heart :diamond :club]]
        (doseq [rank [:2 :3 :4 :5 :6 :7 :8 :9 :10 :jack :queen :king :ace] ]
          (yield {:id (swap! id inc) :suit suit :rank rank}))))))

(pprint (new-deck))

({:suit :spade, :rank :2, :id 1}
 {:suit :spade, :rank :3, :id 2}
 {:suit :spade, :rank :4, :id 3}
 {:suit :spade, :rank :5, :id 4}
 {:suit :spade, :rank :6, :id 5}
 {:suit :spade, :rank :7, :id 6}
 {:suit :spade, :rank :8, :id 7}
 {:suit :spade, :rank :9, :id 8}
 {:suit :spade, :rank :10, :id 9}
 {:suit :spade, :rank :jack, :id 10}
 {:suit :spade, :rank :queen, :id 11}
 {:suit :spade, :rank :king, :id 12}
 {:suit :spade, :rank :ace, :id 13}
 {:suit :heart, :rank :2, :id 14}
 {:suit :heart, :rank :3, :id 15}
 {:suit :heart, :rank :4, :id 16}
 {:suit :heart, :rank :5, :id 17}
<snip>
 {:suit :club, :rank :10, :id 48}
 {:suit :club, :rank :jack, :id 49}
 {:suit :club, :rank :queen, :id 50}
 {:suit :club, :rank :king, :id 51}
 {:suit :club, :rank :ace, :id 52})

I just had to use keywords for the card names since using :rank 13 for the ace (or is it the king?) is just wrong...(shudder)...on so many levels! ;) Note that it is legal for a keyword in Clojure to consist of only digits.

Since the yield is putting the individual items on the output queue, we don't need to return a sequence from for and can just use doseq. Also, we don't need to worry about a multi-variate for, nor about using flatten, concat, or mapcat constructs. Use of the atom for id is as simple as it gets, although you could build the whole thing out of loop/recur instead of the atom & 2 doseq forms if you really wanted to be "pure".

Under the covers lazy-gen/yield uses core.async to create an output stream with a default buffer size of 32.

While for and map-indexed solutions work fine for this example, sometimes it may be clearer to the reader to be extra explicit about the looping constructs, etc. Also, if there were other operations in the inner loop before & after the yield statement, it might be awkward to force the solution into a map or for style solution.

Enjoy!

P.S. Before the purists complain about the use of mutable state & imperative loops, please remember that for some problems (not all!) this may yield a simpler solution with fewer mental stack frames required. Also, the solution is a pure function overall as seen from the outside, since it returns a non-side-effecting lazy-sequence, and neither the state atom nor the imperative loops escape the function body. This is similar to the implementation of many functions in clojure.core itself.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Since the asker tagged their question with the functional-programming tag, I think it would be reasonable to assume that they are looking for a functional solution rather than an imperative one. Under that assumption, this answer is not particularly helpful. \$\endgroup\$ Commented Mar 15, 2017 at 14:03

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.