patternMinor
Tail recursive FizzBuzz in Common Lisp
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
I would call them something different, because what your functions are really doing is not a modulus (there's already a
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
In this function, the indentation you applied is a bit misleading. I added some comments below to illustrate:
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...
Applying above changes as well, I think this would be better:
Optional & default values
As far as I can tell, the only purpose of the
Note: I changed its name to
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".
So in your case,
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
or simpler:
More modern Lisps have a built-in
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:
or using
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)))analyzerThis 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.