HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Tail recursive FizzBuzz in Common Lisp

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
lispfizzbuzzrecursivetailcommon

Problem

I solved FizzBuzz using tail recursion. Is it efficient enough?

(defun mod-3 (n)
   (zerop (mod n 3)))

(defun mod-5 (n)
  (zerop (mod n 5)))

(defun analyzer (n)
  (if (mod-3 n)
  (if (mod-5 n)
      'fizzbuzz
      'fizz)
  (if (mod-5 n)
      'buzz
      n)))

(defun fizzbuzz (limit)
  (fizzbuzz-helper limit nil))

(defun fizzbuzz-helper (limit result)
  (cond ((zerop limit) result)
    (t (fizzbuzz-helper (- limit 1)
            (cons (analyzer limit) result)))))

Solution

When a function does too little...

I personally find that your mod-3 and mod-5 functions do something so very specific that you may as well at least combine them into a generic function.

I would call them something different, because what your functions are really doing is not a modulus (there's already a mod function for that), it's verifying if a number is evenly divisible by another number. I would write something like this:

(defun evenly-divisible-by (n x)
  "Given divisor n and dividend x, returns true if x ÷ n has no remainder."
  (zerop (mod x n)))


analyzer

This function's name really doesn't say much to the next person reading the code. What does it analyze? Why? What goal is it trying to achieve? I think a name like calc-fizzbuzz-for-n would honestly be much more clear about (1) the action "calc" and (2) the expected result from the action "fizzbuzz".

In this function, the indentation you applied is a bit misleading. I added some comments below to illustrate:

(defun calc-fizzbuzz (n)
  (if (evenly-divisible-by 3 n)
  ;then...
  (if (evenly-divisible-by 5 n)
      ;then...
      'fizzbuzz
      ;else...
      'fizz)
  ;else...
  (if (evenly-divisible-by 5 n)
    ;then...
    'buzz
    ;else...
      n)))


This seems minor for just a small indentation (and Lisp can make it seem more trivial since you have all those parentheses anyways), but if you wrote something like this in a more "typical" language, I would certainly expect it would be pointed out during a code review...

if (condA) {
if (condB) {
    foo.actionB();
} else {
    foo.actionA();
}
if (condC) {
    foo.ActionC();
} else {
    foo.defaultAction();
}
}


Applying above changes as well, I think this would be better:

(defun calc-fizzbuzz-for-n (n)
  "Given a number n, return whether it is evenly divisible by 3 (fizz), 5 (buzz), 
  or both (fizzbuzz). Returns n when neither condition is true."
  (if (evenly-divisible-by 3 n)
      (if (evenly-divisible-by 5 n)
          'fizzbuzz
          'fizz)
      (if (evenly-divisible-by 5 n)
          'buzz
          n)))


Optional & default values

As far as I can tell, the only purpose of the fizzbuzz function is to take a limit argument, then call fizzbuzz-helper with the limit and added nil for its result argument. This can be simplified by making the result argument optional, like so:

(defun fizzbuzz (limit &optional result)


Note: I changed its name to fizzbuzz since it is no longer a "helper" to your original fizzbuzz function.

According to a related article on nullprogram.com:

If second and third arguments are not provided, b and c will be bound to nil. To provide a default argument, put that parameter inside a list. Below, when a third argument is not provided, c will be bound to "bar".

(defun foo (a &optional b (c "bar"))


So in your case, result will be nil unless it is provided a different argument. Alternatively, and as it appears to be the intention, you can force result to be nil by default using the let special operator:

(defun fizzbuzz (limit)
  (let (result) ;; defaults to nil if not specified
                ;; you can also do this if you find it more clear: (let ((result nil))
    (cond ;...


Alternative solution using lambda expression

This type of approach is often more idiomatic to Lisp/functional programming. You can use an anonymous function (a.k.a. lambda expression) as an argument to a map function. Your last line would look something like this:

(t (map 'list
        #'fizzbuzz
        (loop for i from 1 to limit by 1 collect i)))


or simpler:

(t (loop for i from 1 to limit collect (fizzbuzz i)))


More modern Lisps have a built-in range function to make things simpler, which Common Lisp doesn't; thankfully though, you can just write your own (code below from Stack Overflow answer Python's range() analog in Common Lisp):

(defun range (max &key (min 0) (step 1))
   (loop for n from min below max by step
      collect n))


And your last line can simply become:

(t (map 'integer #'(lambda (N) (fizzbuzz N)) (range limit 1)))

Thanks to @Renzo for the correction, your last function could be written as:

(defun fizzbuzz (limit) 
  (mapcar #'calc-fizzbuzz-for-n (range (1+ limit) :min 1)))


or using LOOP:

(defun fizzbuzz (limit) 
  (loop for n from 1 upto limit
        collect (calc-fizzbuzz-for-n n)))

Code Snippets

(defun evenly-divisible-by (n x)
  "Given divisor n and dividend x, returns true if x ÷ n has no remainder."
  (zerop (mod x n)))
(defun calc-fizzbuzz (n)
  (if (evenly-divisible-by 3 n)
  ;then...
  (if (evenly-divisible-by 5 n)
      ;then...
      'fizzbuzz
      ;else...
      'fizz)
  ;else...
  (if (evenly-divisible-by 5 n)
    ;then...
    'buzz
    ;else...
      n)))
if (condA) {
if (condB) {
    foo.actionB();
} else {
    foo.actionA();
}
if (condC) {
    foo.ActionC();
} else {
    foo.defaultAction();
}
}
(defun calc-fizzbuzz-for-n (n)
  "Given a number n, return whether it is evenly divisible by 3 (fizz), 5 (buzz), 
  or both (fizzbuzz). Returns n when neither condition is true."
  (if (evenly-divisible-by 3 n)
      (if (evenly-divisible-by 5 n)
          'fizzbuzz
          'fizz)
      (if (evenly-divisible-by 5 n)
          'buzz
          n)))
(defun fizzbuzz (limit &optional result)

Context

StackExchange Code Review Q#122903, answer score: 4

Revisions (0)

No revisions yet.