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?
3 Answers 3
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})))
-
\$\begingroup\$ And an alternative to
apply concat
ismapcat identity
. \$\endgroup\$Chris Murphy– Chris Murphy2017年03月15日 00:48:55 +00:00Commented Mar 15, 2017 at 0:48 -
1\$\begingroup\$ @ChrisMurphy Yes, but I can't see any reason you would want to use
mapcat identity
overapply concat
; it's less readable (at least to me), it's longer, andmapcat
is implemented in terms ofapply concat
anyway, so it should be slower as well. \$\endgroup\$Sam Estep– Sam Estep2017年03月15日 01:13:20 +00:00Commented Mar 15, 2017 at 1:13 -
\$\begingroup\$ All true, yet still its a good thing to know. \$\endgroup\$Chris Murphy– Chris Murphy2017年03月15日 02:17:14 +00:00Commented 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 asmapcat identity
, so it's probably still useful to know the latter when reading other people's code. \$\endgroup\$Sam Estep– Sam Estep2017年03月15日 02:23:05 +00:00Commented Mar 15, 2017 at 2:23
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)))))
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.
-
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\$Sam Estep– Sam Estep2017年03月15日 14:03:23 +00:00Commented Mar 15, 2017 at 14:03
Explore related questions
See similar questions with these tags.