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.

Help me make it better. Requirement: support arbitrary number of factors, not just 3 & 5.

(ns fizzbuzz.core
  (:use [clojure.contrib.string :only (join)]))

(def targets (sorted-map 3 "fizz" 5 "buzz" 7 "baz"))

;; match any applicable factors
(defn match [n, factors]
  (filter #(= 0 (mod n %)) factors))

(defn fizzbuzz [n]
  (let [factors (keys targets)
        matches (match n factors)]
    (if (= 0 (count matches))
      (str n)
      (reduce str (map targets matches)))))

(join ", " (map fizzbuzz (range 1 20)))
share|improve this question
add comment

1 Answer

I tried keeping your approach, although I would've made it an infinite lazy sequence. Yours is probably a better idea, since you still have the infinite sequence (mapping to range as you did) and you can map it to other non-linear sequences too.

(def targets  ;; This is more readable for me
  (sorted-map
    3 "fizz",
    5 "buzz",
    7 "baz"))

(defn fizzbuzz [targets n]  ;; Pass the targets here! Commas aren't very common in args lists
  (let [matches (keep (fn [[f w]]  ;; Destructuring is nice :P
                        (when (zero? (mod n f)) w))  ;; Got rid of your helper function
                      targets)]  ;; ^ Use 'zero?' instead of '(= 0 ...)'
    (if (empty? matches)  ;; Use 'empty?' instead of '(= 0 (count ...))'
      (str n)
      (apply str matches))))  ;; I prefer apply here, but seen both

;;;;; EXAMPLE
(map (partial fizzbuzz targets) (range 1 20))
   => ("1" "2" "fizz" "4" "buzz" "fizz" "baz" "8" "fizz" "buzz" "11" "fizz" "13" "baz" "fizzbuzz" "16" "17" "fizz" "19" "buzz")

There's an even more idiomatic way to rewrite the (if (empty? matches)...) bit:

(if matches
  (apply str matches)
  (str n))

Lazy sequence style. Consider fizzbuzz and targets are already defined:

(defn lazy-fizzbuzz
  ([targets n]
    (lazy-seq
      (cons (fizzbuzz targets n)
            (lazy-fizzbuzz targets (inc n)))))
  ([targets]
    (lazy-fizzbuzz targets 1)))

;;;;; EXAMPLES
(take 20 (lazy-fizzbuzz targets))
   => ("1" "2" "fizz" "4" "buzz" "fizz" "baz" "8" "fizz" "buzz" "11" "fizz" "13" "baz" "fizzbuzz" "16" "17" "fizz" "19" "buzz")

;; You can also specify an initial n
(take 20 (lazy-fizzbuzz targets 20))
   => ("buzz" "fizzbaz" "22" "23" "fizz" "buzz" "26" "fizz" "baz" "29" "fizzbuzz" "31" "32" "fizz" "34" "buzzbaz" "fizz" "37" "38" "fizz")

;; which is the same as...
(take 20 (drop 19 (lazy-fizzbuzz targets)))
   => ("buzz" "fizzbaz" "22" "23" "fizz" "buzz" "26" "fizz" "baz" "29" "fizzbuzz" "31" "32" "fizz" "34" "buzzbaz" "fizz" "37" "38" "fizz")
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.