5
\$\begingroup\$

I've started learning Clojure, after many years of Java. As one of a series of exercises, I'm writing a simple card game, and the overhand-shuffle function is shown below.

My main concerns are:

  1. Most of my code appears inside the let, and to me it looks more procedural than functional, is this the best way to do this? Please bear in mind that the reasons I did it this way are:

    • I wanted to avoid having the body of the function as one big, monolithic block of dense, unreadable code
    • Binding lots of symbols like this chops the code into manageable pieces, making it easier for me to read and understand intermediate steps
    • Debugging is also easier, as I can examine what's happening at each step
  2. In my reduce, I'm passing back two values on each iteration (stored in the map result). These are :cards (which contains cards not yet allocated) and :new-cards (which builds up the new version of shuffled cards). However, I'm only interested in the new, shuffles cards being passed back as the result of the reduce, not the intermediate results that reduce needs at each step. Is there a cleaner way to do this?

(defn overhand-shuffle
  "Shuffle the cards, using the overhand shuffle technique.
  REFL https://en.wikipedia.org/wiki/Shuffling#Overhand_shuffle"
  [cards]
  (let [number-of-cards   (count cards)
        number-of-slices  (/ (* number-of-cards 7) 52)
        max-slice-size    (int (/ (* number-of-cards 0.6) number-of-slices))

        ; Randomly determine the size of the little slices we will take
        slice-sizes
          (for [_i (range number-of-slices)]
            (inc (rand-int max-slice-size))
            )

        slice-total       (apply + slice-sizes)
        top-card-count    (- number-of-cards slice-total)

        ; Take the top cards, and make slices out of the remaining cards
        top-cards         (take top-card-count cards)
        slice-cards       (drop top-card-count cards)

        ; Chop up the slices, and return with them in reverse order
        slices
          (reduce
            (fn [result slice-size]
              (let [cards     (:cards result)
                    new-cards (:new-cards result)

                    ; Take a slice, being the first n of the remaining cards
                    slice     (take slice-size cards)
                    ]
                ; Return with the remaining cards and the growing list of new cards
                {:cards     (drop slice-size cards)
                 :new-cards (concat slice new-cards)}
                )
              )
            ; initial results we will start the reduce with
            {:cards slice-cards :new-cards nil}

            ; iterate over the list of slice sizes
            slice-sizes)
        ]
    ; We have what we need, so return with the slices on top of the remaining cards
    (concat
      ; The slices that came from the bottom of the cards
      (:new-cards slices)

      ; Add what used to be the top cards, after the slices
      top-cards
      )
    )
  )

(defn test-shuffle
  "Simple invocation of shuffle, to show result"
  []
  (let [simple-cards (range 1 53)
        shuffle-times 10]
    (loop [count shuffle-times
           cards simple-cards]
      (if (< count 0)
        cards
        (recur (dec count) (overhand-shuffle cards))
        )
      )
    )
  )
\$\endgroup\$
2
\$\begingroup\$

A quick disclaimer: I am not really a clojure developer so take any of my recommendations with a pinch of salt.

Regarding your giant let: I don't think that's necessarily a bad thing to do, but in some cases it might be worth splitting some of it off into separate functions. For example max-slice-size and number-of-slices are only really needed to generate slice-sizes, so it may be worth splitting them off into a make-slice-sizes function or something.

Most of the reasons you listed for having things in a giant let are actually really good reasons to use smaller functions:

  • You've got a selection of smaller functions rather than the big dense mono-function you were trying to avoid.
  • Your individual functions are your manageable pieces, rather than each binding of the let. You can call them from a repl, or write tests of just those individual functions to make sure they are behaving as expected.
  • Debugging is also still possible, but with the benefit that it's easier to use a repl & write tests.

A few points about your reduce:

  • You can simplify it a bit by returning a vector instead of a map and using argument destructuring instead of getting the values out of the map.
  • When you want to do a take & a drop using the same numbers it's usually easier to use split-at.
  • Passing back 2 things from a reduce doesn't seem wrong. However I would consider just using your reduce to build up a list of slices that you can manipulate afterwards rather than using concat to actually do the shuffling within the reduce. The reductions function can help with this - it is like reduce, but it returns the intermediate steps that you can manipulate later.
  • Currently you are building up a list of random slice sizes, and then writing some more code to handle the remainder of the cards (top-cards in your code). I think you could simplify your code a fair bit by just treating this as another slice and adding it in to your slice-sizes sequence.

I've had a go at implementing my suggestions, and ended up with this:

(defn overhand-shuffle
  "Shuffle the cards, using the overhand shuffle technique.
  REFL https://en.wikipedia.org/wiki/Shuffling#Overhand_shuffle"
  [cards]
  (let [slice-sizes        (make-slice-sizes (count cards))
        ; The first value returned by reductions is the initial value, so we
        ; drop it here.
        [_ & slices]       (map first
                             (reductions (fn [[_ remaining-cards] slice-size]
                                           (split-at slice-size remaining-cards))
                                         [nil cards]
                                         slice-sizes))]
    (apply concat (reverse slices))))

(defn- make-slice-sizes
  "Randomly determine the size of the little slices we will take"
  [number-of-cards]
  (let [number-of-slices  (/ (* number-of-cards 7) 52)
        max-slice-size    (int (/ (* number-of-cards 0.6) number-of-slices))
        slice-sizes       (map inc (repeatedly
                                    number-of-slices
                                    #(rand-int max-slice-size)))
        remaining-cards   (- number-of-cards (apply + slice-sizes))]
    ;; We need to make sure all our cards get "shuffled", so add a slice of the
    ;; remaining cards on the end of our list of slices
    (concat slice-sizes [remaining-cards])))

Please let me know if I've explained any of this badly, and I'll try and improve it.

Edit: You can actually take this a bit further and split out your reduction into a function as well:

(defn build-slices
  "Builds a sequence of slices from cards.
   The sizes of the slices are determined using slice-sizes"
  [cards slice-sizes]
  ;; Perform a reduction where the first element of each step is the slice,
  ;; and the second element is the remaining cards.  Use a let binding to get
  ;; rid of the first reduction, because it is just the initial value we
  ;; passed in.
  (let [[_ & slices]  (map first
                          (reductions (fn [[_ remaining-cards] slice-size]
                                        (split-at slice-size remaining-cards))
                                      [nil cards]
                                      slice-sizes))]
    slices))

Which allows you to simplify your overhand-shuffle function further:

(defn overhand-shuffle
  "Shuffle the cards, using the overhand shuffle technique.
  REFL https://en.wikipedia.org/wiki/Shuffling#Overhand_shuffle"
  [cards]
  (let [slice-sizes   (make-slice-sizes (count cards))
        slices        (build-slices cards slice-sizes)]
    (apply concat (reverse slices))))

Since both these functions are now just a series of steps each building on the last you could rewrite using one of the threading macros (-> or ->>). These let you list a series of forms and automatically insert the result of the last form into the next form, as either the first (for ->) or the last (for ->>) parameter to each of them. This allows you to make your function a series of "steps" that transform your input data, a bit like your original let bindings, but without the intermediate variables:

(defn build-slices
  "Builds a sequence of slices from cards.
   The sizes of the slices are determined using slice-sizes"
  [cards slice-sizes]
  (->> slice-sizes
       ;; Perform a reduction where the first element of each step is the slice,
       ;; and the second element is the remaining cards.
       (reductions (fn [[_ remaining-cards] slice-size]
                     (split-at slice-size remaining-cards))
                   [nil cards])
       ;; Drop the first element, as it is just our initial [nil, cards] value.
       (drop 1)
       ;; Extract each of the slices.
       (map first)))

(defn overhand-shuffle
  "Shuffle the cards, using the overhand shuffle technique.
  REFL https://en.wikipedia.org/wiki/Shuffling#Overhand_shuffle"
  [cards]
  (->> cards
       (count)
       (make-slice-sizes)
       (build-slices cards)
       (reverse)
       (apply concat)))

A bonus edit: the loop in your test-shuffle function is very procedural. Could be better with iterate:

(defn test-shuffle
  "Simple invocation of shuffle, to show result"
  []
  (let [simple-cards (range 1 53)
        shuffle-times 10]
    (nth (iterate overhand-shuffle simple-cards) shuffle-times)))
\$\endgroup\$
  • \$\begingroup\$ Wow! Thanks for that. There's such a huge number of ideas here like argument destructuring, private functions, and functions like reductions and split-at that I wasn't aware of. Thank you again!! \$\endgroup\$ – Steve Moseley Jan 14 '16 at 23:47
  • \$\begingroup\$ @SteveMoseley no problem - I actually enjoyed putting this together, don't get to use clojure anywhere near as much as I'd like. \$\endgroup\$ – obmarg Jan 15 '16 at 0:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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