Return Set<Integer> with random unique Integer numbers between min and max range

Question

I need set of random unique Integer numbers for my unit test. I implemented this method, however I'm not sure if this is best way to implement this.

private Set<Integer> getRandomUniqueNumberSet(Integer min, Integer max)
{
    Set<Integer> added = new LinkedHashSet<Integer>(); 
    for(int i = min; i <= max; i++)
    {
        Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
        Boolean loop = false; 
        if(!added.add(index)) loop = true;
        while(loop)
        {
            index = ThreadLocalRandom.current().nextInt(min, max + 1);
            if(added.add(index)) loop = false;
        }
    }
    return added;
}

Example:

for(Integer index : getRandomUniqueNumberSet(2, 15))
{
    System.out.println("Index: " + index);
}

Results:

Index: 8
Index: 5
Index: 15
Index: 9
Index: 4
Index: 3
Index: 7
Index: 2
Index: 11
Index: 14
Index: 6
Index: 10
Index: 13
Index: 12

Answer 1

Generating unique random numbers within a range

A good way to generate unique random numbers within a range is to create a list with the desired unique numbers, and then shuffle it.

  private Set<Integer> getRandomUniqueNumberSet(int min, int max) {
    List<Integer> numbers = IntStream.rangeClosed(min, max).boxed().collect(Collectors.toList());
    Collections.shuffle(numbers);
    return new LinkedHashSet<>(numbers);
  }

Don't used boxed types when you don't need null values

This method takes Integer parameters, which may be null:

private Set<Integer> getRandomUniqueNumberSet(Integer min, Integer max)

But the implementation doesn't handle the case when these values are null. And it doesn't make sense to support such ranges. Change those types to primitive int.

The same goes for the local variables Integer index and Boolean loop.

Why ThreadLocalRandom?

As per the javadoc, "use of ThreadLocalRandom is particularly appropriate when multiple tasks use random numbers in parallel in thread pools". I doubt that's necessary in your use case, in which case I suggest to use an instance of Random instead.

Answer 2

The code block

Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
Boolean loop = false; 
if(!added.add(index)) loop = true;
while(loop)
{
    index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if(added.add(index)) loop = false;
}

Contains several lines of duplicated code. This pattern can often be simplified by removing the code outside of the while block:

Boolean success = false; 
while (!success)
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if (added.add(index))
        success = true;
}

or a do/while block, which is more idiomatic for code that needs to execute at least once:

Boolean success = false;
do
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    success = added.add(index); // note: this is simpler than the if version
}
while (!success);

I'm not a huge fan of while (true), but some people might prefer the simpler:

while (true)
{
    Integer index = ThreadLocalRandom.current().nextInt(min, max + 1);
    if (added.add(index))
        break;
}

This is all if you want to simply improve the existing structure. janos is correct that if all you want is a shuffling of a range of numbers you should approach it from that direction. Your current solution can take an unbounded amount of time to run, if the random number generator is "unlucky". You are returning a Set, which generally doesn't have a predictable order. You happen to use an implementation that does preserve insert order, but callers of you code have now way of knowing that. I would return a data structure that implies ordering, like a List. Internally you used a Set to have it prevent duplicates, but if you shuffle that's not functionality you need.

A final nit: added isn't a great name for the set that you are going to return. I often use ret to name the variable that will be the return value, though some might prefer a more descriptive name like set.