3
\$\begingroup\$

I have this method that will give a random prime for a given range (min inclusive an max not inclusive) all comments are welcome:

public static int randomPrime(int min, int max) {
    if (min < 0)
        throw new IllegalArgumentException("min must be positive.");
    if (min >= max)
        throw new IllegalArgumentException("min must be smaller the max.");
    if (!containsPrime(min,max))
        throw new IllegalArgumentException("no Primes in this interval.");
    if (rand == null) {
        rand = new Random();
    }
    int out = rand.nextInt(max - min) + min;
    while (!isPrime(out)) {
        out = rand.nextInt(max - min) + min;
    }
    return out;
}
    private static boolean containsPrime(int min, int max) {
    if (isPrime(min)||isPrime(max-1)){
        return true;
    }
    if(min%2==0){
        min +=1;
    }
    while (min<max){
        if (isPrime(min)){
            return true;
        }
        min +=2;
    }
    return false;
}
\$\endgroup\$
5
  • \$\begingroup\$ Would you like to share your isPrime method as well? \$\endgroup\$
    – Adam
    Commented Dec 28, 2012 at 14:51
  • \$\begingroup\$ If this is for cryptography purposes (where random prime numbers are used a lot), you may want to have your random number generator be injectable (Random may not be good enough). You're vulnerable to integer overflow; depending on the starting values of min/max (and the implementation of isPrime()), you could spin through all negative numbers. There's probably at least one additional optimization for the prime range-check, something about 3 prime numbers every 100 or something (I can't remember exactly what it was, though). \$\endgroup\$
    – Clockwork-Muse
    Commented Dec 28, 2012 at 22:21
  • \$\begingroup\$ @Clockwork-Muse I think you missed my input validation, the code is pure for statistics, no worries about Random i am using a specialised version there. \$\endgroup\$
    – Frank
    Commented Dec 28, 2012 at 23:28
  • 1
    \$\begingroup\$ Argh, I forgot that Integer.MAX_VALUE is odd - if it were even, you would be. Other things - the even/odd check can be performed first, before the initial check for primeness of min/max. Move min += 2; above the prime check in the while (min < max) loop - you check the 'initial' min value twice. \$\endgroup\$
    – Clockwork-Muse
    Commented Dec 29, 2012 at 0:17
  • \$\begingroup\$ @Clockwork-Muse Integer.MAX_VALUE = 2147483647, is not just odd... it's a prime \$\endgroup\$
    – Frank
    Commented Dec 29, 2012 at 13:07

2 Answers 2

Reset to default
3
\$\begingroup\$

First of all, use some white space. The scrunched expressions are harder to read. At a minimum, I recommend spaces around every operator.

In containsPrime, if the variable min is odd, then it gets checked twice. You can replace the second if block with this:

if (min % 2 == 0) min += 1;
else min += 2;
\$\endgroup\$
3
  • 1
    \$\begingroup\$ and while we're at it, min += 1 can and should be min++ \$\endgroup\$
    – Adam
    Commented Dec 28, 2012 at 16:28
  • \$\begingroup\$ That if-else statement can be reduced to a ternary to save a line: min += (min % 2 == 0) ? 1 : 2;. \$\endgroup\$
    – MrLore
    Commented Dec 28, 2012 at 20:23
  • \$\begingroup\$ I am not a fan of ternary's so i'll stick to the suggestion of Donald on this one, but Thx,to both of you for the constructive feedback. \$\endgroup\$
    – Frank
    Commented Dec 28, 2012 at 21:42
4
\$\begingroup\$ 
int out = rand.nextInt(max - min) + min;
while (!isPrime(out)) {
    out = rand.nextInt(max - min) + min;
}
return out;

This duplicate code can be replaced with a do/while loop. The rule of thumb is, if you always need to perform an action at least once, do while!

int out;
do {
    out = rand.nextInt(max - min) + min;
} while(!isPrime(out));
return out;
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.