Generating a not-the-same-as-last-time random number

Question

I have to generate random number which differs from the number generated last time.

Which of these two variants is better? Or maybe you can suggest better implementation.

int generateRandom() {
    int randomNumber;
    do {
        randomNumber = random.nextInt(UPPER_BOUND);
    } while (randomNumber == lastRandomNumber);
    lastRandomNumber = randomNumber;
    return randomNumber;
}

vs

int generateRandom() {
    while (true) {
        int randomNumber = random.nextInt(UPPER_BOUND);
        if (randomNumber != lastRandomNumber) {
            lastRandomNumber = randomNumber;
            return randomNumber;
        }
    }
}

Answer 1

Neither option is great. Looking at the second option first (because it is the worst, in my opinion):

int generateRandom() {
    while (true) {
        int randomNumber = random.nextInt(UPPER_BOUND);
        if (randomNumber != lastRandomNumber) {
            lastRandomNumber = randomNumber;
            return randomNumber;
        }
    }
}

This contains a seemingly infinite loop. while(true) is often an indication that there's a better way to do things (not necessarily always, but in this case, yes). The reason it is a problem here is because it gives the impression that the loop repeats often, when, in fact, it seldom repeats ever.

The first block is better:

int generateRandom() {
    int randomNumber;
    do {
        randomNumber = random.nextInt(UPPER_BOUND);
    } while (randomNumber == lastRandomNumber);
    lastRandomNumber = randomNumber;
    return randomNumber;
}

This is better because the loop-terminating condition is much cleaner, more visible. The logic-path through the method is traditional. Still, it's not great. I dislike the need to declare the randomNumber variable as a separate declaration.

The 'while loop' solution would be more readable (even though it duplicates the nextInt() call), as:

int generateRandom() {
    int randomNumber = random.nextInt(UPPER_BOUND);
    while (randomNumber == lastRandomNumber) {
        randomNumber = random.nextInt(UPPER_BOUND);
    }
    lastRandomNumber = randomNumber;
    return randomNumber;
}

Even with the code duplication, it makes it clear that the normal case is handled, and then the while loop is a recovery process.

@Pimgd suggested sending the lastRandomNumber in as a parameter, and @maartinus suggested a single if-block condition rather than a while loop solution. In principle these are both good ideas. There is a way to make it completely reentrant, and also a way to remove the while-loop and the id-conditionals. My solution would be:

int generateRandom(int lastRandomNumber) {

    // add-and-wrap another random number to produce a guaranteed
    // different result.
    // note the one-less-than UPPER_BOUND input
    int rotate = 1 + random.nextInt(UPPER_BOUND - 1);
    // 'rotate' the last number
    return (lastRandomNumber + rotate) % UPPER_BOUND;

}

@mjolka pointed out that if UPPER_BOUND is large, it is possible for the values to overflow in the sum, and that the better solution would be:

return ((lastRandomNumber + rotate) & Integer.MAX_VALUE) % UPPER_BOUND;

Answer 2

I'd say both of them could use an improvement:

You use a class member variable lastRandomNumber. This means your functions are no longer thread-safe. Consider passing it in as an argument...

Next, you can use a trick used for reading files: while((line = nextLine()) != null). What that does is "set line to the next line, then check if line is not null."

In your case, this would be

while((randomNumber = random.nextInt(UPPER_BOUND)) != lastRandomNumber){}

Making the result

int generateRandom(int lastRandomNumber){
    int randomNumber;
    while((randomNumber = random.nextInt(UPPER_BOUND)) != lastRandomNumber){}
    return randomNumber;
}

There's a version without a loop too:

int generateRandom(int lastRandomNumber){
    int randomNumber = random.nextInt(UPPER_BOUND - 1) + 1;
    if(randomNumber == lastRandomNumber){
        randomNumber = 0;
    }
    return randomNumber;
}

... and as a last nitpick, I think you should rename the method to be generateRandomThatIsNot(int lastRandomNumber).

Answer 3

Neither? Both look fine. I'd just rename randomNumber to something else (e.g., result) as I found similarly names bad.

You surely know, that it never terminates for UPPER_BOUND == 1, but as this is a constant, there's no need for a check.

You could also optimize it to

int generateDifferentRandom() {
    // There's one less possible result, note the argument to random.
    int result = random.nextInt(UPPER_BOUND - 1);
    if (result == lastRandomNumber) {
        result = UPPER_BOUND - 1;
    }
    lastRandomNumber = result;
    return result;
}

but I'm not suggesting it. Your code looks cleaner, I just wanted to show an alternative.

---

Concerning thread-safety mentioned by Pimgd, it's surely something you should keep in mind. Passing lastRandomNumber as an argument solves it nicely, but often objects do need their own state. And most of them don't need to be thread-safe as they don't get used from multiple threads. You could simply add synchronized or use AtomicInteger#compareAndSet to obtain thread-safety, but usually, you don't have to.