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;
randomNumber
to be declared somewhere. Obviously, it should be declared in the method itself. \$\endgroup\$ – maaartinus Oct 14 '14 at 10:36