8
\$\begingroup\$

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;
        }
    }
}
\$\endgroup\$
7
  • \$\begingroup\$ This code is verified to work correctly? \$\endgroup\$ – Pimgd Oct 14 '14 at 10:28
  • \$\begingroup\$ @Pimgd yes. Why do you ask? What's wrong with this code snippet? \$\endgroup\$ – Leonid Semyonov Oct 14 '14 at 10:30
  • \$\begingroup\$ It's vital for my answer. \$\endgroup\$ – Pimgd Oct 14 '14 at 10:30
  • 2
    \$\begingroup\$ @LeonidSemyonov Your second snippet needs randomNumber to be declared somewhere. Obviously, it should be declared in the method itself. \$\endgroup\$ – maaartinus Oct 14 '14 at 10:36
  • \$\begingroup\$ @maaartinus I corrected the snippet. \$\endgroup\$ – Leonid Semyonov Oct 14 '14 at 10:43
13
\$\begingroup\$

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;
\$\endgroup\$
2
  • \$\begingroup\$ Aaand he solved it. There you go, a one liner solution if you replace rotate with its value. I salute thee. \$\endgroup\$ – Pimgd Oct 14 '14 at 12:00
  • 2
    \$\begingroup\$ body; while (condition) { body; } defeats the whole point of the do { body } while (condition); language feature. I'd say that it's definitely worse than both of the originals. \$\endgroup\$ – 200_success Oct 15 '14 at 16:21
4
\$\begingroup\$

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).

\$\endgroup\$
2
  • \$\begingroup\$ I hate this trick... I'd call it obfuscation. It's a leftover from times, when compilers were dumb. \$\endgroup\$ – maaartinus Oct 14 '14 at 10:39
  • \$\begingroup\$ In languages that allow implicit casting, there's an even nastier trick: value = random(bound-1)+1; return value*(value!=lastValue);. \$\endgroup\$ – Pimgd Oct 14 '14 at 10:58
4
\$\begingroup\$

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.

\$\endgroup\$
3
  • 3
    \$\begingroup\$ This solution will tend to return UPPER_BOUND - 1 as a value more often than any other value, which makes the randomness wrong. \$\endgroup\$ – rolfl Oct 14 '14 at 10:39
  • 1
    \$\begingroup\$ @rolfl I haven't tested it... but have you noticed the upper bound I'm passing to the random call? Because of it, UPPER_BOUND-1 will be returned only in case of collisions. It can't be returned twice in a row as it can't be generated by random.nextInt(UPPER_BOUND - 1). Note that I clearly said, I'm not recommending this. It's correct, but as we can see, pretty unclear. Btw., it's a common trick for returning constrained random numbers. \$\endgroup\$ – maaartinus Oct 14 '14 at 10:50
  • \$\begingroup\$ Ahh, I see where I went wrong, you should make it more clear that the upperbound-1 is the input to the nextInt, as it is not immediately obvious that it is, and the consequences of that subtlety are significant. \$\endgroup\$ – rolfl Oct 14 '14 at 11:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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