-
Notifications
You must be signed in to change notification settings - Fork 0
Open
Description
Hi Alessio, thanks for the article A pure functional Primality Test in Scala
I propose for consideration, with regards to:
def isPrime(n: Int)(implicit i: Int = 5): Boolean =
{
if (n <= 3)
return n > 1
if (n % 2 == 0 || n % 3 == 0)
return false
while (i * i <= n)
{
if (n % i == 0 || n % (i + 2) == 0)
return false
return isPrime(n)(i + 6)
}
true
}That:
- The condition
(n <= 3):
1.1. Needs to be evaluated at most once. - The condition
(n % 2 == 0 || n % 3 == 0):
2.1. Needs to be evaluated at most once.
2.2. The is-even test(n % 2 == 0)is likely more efficient as((n & 1) == 0), but a JMH micro-benchmark would be required to confirm this. - The statement
while (i * i <= n):
3.1. The while-statement executes at most once, so can be replaced by an if-statement.
3.2.Int.MaxValueis prime, buti*iwill eventually overflowInt, become negative, incorrectly satisfyi * i <= nand declareInt.MaxValuenot prime. (That's when tail-recursive, the current implementation throws aStackOverflowErrorforisPrime(Int.MaxValue)before getting that far.)
3.3.i * i <= ncan be replaced byi <= sqrt(n), wheresqrt(n)is evaluated once andIntoverflow is avoided. - The repetitive component of the primality test can be marked tail-recursive for further compiler optimisation.
4.1. There are differing styles for defining the recursive function, as already mentioned in the comments so far. But avoiding default parameters, implicits and nested functions with simple private scope is probably sufficient.
def isPrime(n: Int): Boolean =
if (n <= 3) n > 1
else if ((n & 1) == 0 || n % 3 == 0) false
else isPrime(n, 5, Math.sqrt(n).floor.toInt)
@scala.annotation.tailrec
private def isPrime(n: Int, i: Int, maxI: Int): Boolean =
if (i <= maxI) {
if (n % i == 0 || n % (i + 2) == 0) false
else isPrime(n, i + 6, maxI)
} else trueCorrections and further optimisations welcome.
Thanks
Kind regards,
Greg
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
No labels