Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

2 Answers

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
(map #(cond (zero? (mod % 15)) "FizzBuzz" (zero? (mod % 3)) "Fizz" (zero? (mod % 5)) "Buzz" :else %) (range 1 101))
share|improve this answer
1  
This is a hard-coded answer; does not seem to be what OP wanted. – Leonid Jul 14 '12 at 1:01
3  
Hello mca. Please take a look at the page "How to Answer". A code review without an explanation really isn't a code review at all - in my opinion, even if the OP was only asking you to "make it better". – codesparkle Jul 14 '12 at 8:18

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.