Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
}
share|improve this question
    
Would you like to share your isPrime method as well? –  codesparkle Dec 28 '12 at 14:51
    
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). –  Clockwork-Muse Dec 28 '12 at 22:21
    
@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. –  Frank Dec 28 '12 at 23:28
1  
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. –  Clockwork-Muse Dec 29 '12 at 0:17
    
@Clockwork-Muse Integer.MAX_VALUE = 2147483647, is not just odd... it's a prime –  Frank Dec 29 '12 at 13:07

2 Answers 2

up vote 2 down vote accepted

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;
share|improve this answer
1  
and while we're at it, min += 1 can and should be min++ –  codesparkle Dec 28 '12 at 16:28
    
That if-else statement can be reduced to a ternary to save a line: min += (min % 2 == 0) ? 1 : 2;. –  MrLore Dec 28 '12 at 20:23
    
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. –  Frank Dec 28 '12 at 21:42
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;
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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