Trouble with defining an ISPRIME function

Question

I am very new to LISP and was working through some beginner problems. I tried defining an ISPRIME function but it doesn't seem to be working correctly. Here is my code:

 (defun ISPRIME (n &optional (d (- n 1))) 
      (if (= d 0)
           ( return-from ISPRIME t )) 
      (if (= (mod n d) 0)
           ( return-from ISPRIME nil )) 
      (ISPRIME n (- d 1)))

But upon running my code I use the value 5 as an example:

 (ISPRIME 5)
 Nil

5 should be a prime number. I suspect that everything is falling into the: (if (= (mod n d) 0) statement when it shouldn't be. d should continue to decrement until reaching 0 and returning true, yet it doesn't. I can't seem to see where my logic error is occuring.

Any and all help is greatly appreciated!

`(mod 5 1)`. Also, you should use [`COND`](http://www.lispworks.com/documentation/HyperSpec/Body/m_cond.htm) rather than `IF`+`RETURN-FROM`s. — jkiiski, Sep 16 '16 at 04:41

Renzo · Accepted Answer · 2016-09-16T07:28:12.083

There is an error in your code: the function should stop at 1, not at 0, since any number is divisible by 1. Simply change the function is this way:

(defun ISPRIME (n &optional (d (- n 1))) 
      (if (= d 1)                         ; <- change from 0 to 1
           ( return-from ISPRIME t )) 
      (if (= (mod n d) 0)
           ( return-from ISPRIME nil )) 
      (ISPRIME n (- d 1)))

But note that your function is not very efficient, since return-from returns from the last call of the function, not from the “recursion tower”, and so your function can be rewritten by eliminating it in this way, by using the more compact “conditional” cond, instead of if, and substituting return-from with the result:

(defun isprime (n &optional (d (- n 1)))
  (cond ((= d 1) t)
        ((= (mod n d) 0) nil)
        (t (isprime n (- d 1)))))

This is still recursive, but more idiomatic for Common Lisp. Of course one could transform this function in a more efficient iterative version, and apply a more efficient algorithm. Note, however, that a smart compiler will transform automatically this function in iteration, due to the fact that the function is tail recursive.

Added

You can also add an initial check that the parameter n is greater than 1, for instance:

(defun isprime (n &optional (d (- n 1)))
  (cond ((<= n 1) nil)
        ((= d 1) t)
        ((= (mod n d) 0) nil)
        (t (isprime n (- d 1)))))

coredump · Answer 2 · 2016-09-18T08:53:47.853

2

Since you are computing a boolean, instances of (if test T NIL) can be replaced by test and more generally, the result can probably be expressed as a single boolean expression. I tend to find them more readable. Here is a little modification of the accepted answer as an example:

(defun is-prime (n &optional (d (1- n)))
  (or (= d 1)
      (and (plusp d)
           (plusp (mod n d))
           (is-prime n (1- d)))))

For completeness, and based on the answer from Will Ness, here is a LOOP version:

(defun is-prime (n)
  (loop
     for d = 2 then (+ d add)
     for add = 1 then 2
     thereis (> (* d d) n)
     until (= (mod n d) 0)))

And an equivalent DO version:

(defun is-prime (n)
  (do ((d 2 (+ d add))
       (add 1 2))
      ((> (* d d) n) t)
    (when (zerop (mod n d))
      (return nil))))

edited Sep 18 '16 at 08:53

answered Sep 16 '16 at 07:24

coredump

37,664
5
43
77

[`loop-finish`](http://clhs.lisp.se/Body/m_loop_f.htm) (which is triggered by `until`, AFAICS) doesn't say explicitly what's returned when there's no value being accumulated, and no loop epilogue clauses are defined. obviously it returns NIL, but, is there any place in CLHS where it explicitly says so, do you know? – Will Ness Sep 17 '16 at 22:40
@WillNess The most relevant spec part would be "*Unless some other clause contributes a return value, the default value returned is nil.*" in [6.1.4 Termination Test Clauses](http://www.lispworks.com/documentation/lw51/CLHS/Body/06_ad.htm), for the `thereis` clause. This particular combination looks well-defined for me, but I would certainly like to have a more formal guarantee. – coredump Sep 18 '16 at 05:56
1

I guess... thanks for the quote/link. or a final clause `finally (return NIL)` could be added just for the sake of being explicit. same with `(return-from is-prime)`, which could be written `(return nil)` (using `return` also helps if one would want to rename the function... :) ). YMMV. :) – Will Ness Sep 18 '16 at 08:06

Will Ness · Answer 3 · 2016-09-17T23:52:10.253

You shouldn't use (tail) recursion, as Common Lisp has no tail call optimization guarantee. Use do or loop instead, or even prog with go.

And algorithmically, always test your potential divisors in increasing order, starting from 2, and stop when you exceed (sqrt n):

(defun ISPRIME (n) 
  (prog ((d 2))                          ; defines implicit block named NIL
    LOOP
      (if (> (* d d) n)
           ( return-from ISPRIME t ))    ; (return T) also works
      (if (= (mod n d) 0)
           ( return-from ISPRIME nil ))  ; or just (return) 
      (if (= d 2)
        (incf d 1)
        (incf d 2))
      (go LOOP)))

(return) is the same as (return nil), and (return val) is the same as (return-from NIL val). Since prog defines an implicit block named NIL, shorter, and more general, calls to return can be used there instead.

A fun way to pursue here is to use an extendable list of primes created by filtering an increasing numbers supply with this isprime function, to be used as divisors supply in another isprime-p function which would only test its argument by these primes, and not all odds, thus achieving another algorithmic performance gain. The primes list should be extended as needed. The primes would only go upto the square root of the number being tested, and the primes themselves would only need to be tested by numbers upto the square root of the top prime (so, fourth root of the number being tested).

Trouble with defining an ISPRIME function

3 Answers3