patternjavaMinor
Iterator producing unique random numbers in a specified range
Viewed 0 times
randomuniqueiteratorproducingrangenumbersspecified
Problem
I needed an iterator that produced random numbers, never repeating them, in a specified range of 0..max-1 (index for a collection). When the numbers are exhausted it should not have a next anymore.
The simplest way might be to shuffle an ArrayList and make an iterator. But in typical usage, I will want only 4-5 unique random numbers, from a total in the region of 100. So shuffle seemed too CPU-heavy. And here is the class I came up with. It is written in Java 8 (the requirement of version 8 is acceptable in my case).
I would appreciate comments on this code.
The simplest way might be to shuffle an ArrayList and make an iterator. But in typical usage, I will want only 4-5 unique random numbers, from a total in the region of 100. So shuffle seemed too CPU-heavy. And here is the class I came up with. It is written in Java 8 (the requirement of version 8 is acceptable in my case).
import java.util.Iterator;
import java.util.List;
import java.util.Random;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
public class NonRepeatingRandom implements Iterator {
private List unused;
private Random random;
public NonRepeatingRandom(int max) {
unused = IntStream.range(0, max).boxed().collect(Collectors.toList());
random = new Random();
}
@Override
public boolean hasNext() {
return ( unused.size() > 0 );
}
@Override
public Integer next() {
int size = unused.size();
if ( size==0 ) {
throw new java.util.NoSuchElementException();
}
int idx = random.nextInt(size);
int result = unused.get(idx);
unused.remove(idx);
return result;
}
}I would appreciate comments on this code.
Solution
Looks pretty reasonable. Just a few minor issues:
That's fine for general-purpose use, but you should consider adding a constructor which takes
I generally recommend favouring
This is the biggest problem, although for your typical use case it might be relatively minor because it doesn't sound like this iterator is going to be a bottleneck.
Removing a random element from a list takes \$O(n)\$ time. But since you don't care about the order of the elements in the list, you could copy the last element to position
public NonRepeatingRandom(int max) {That's fine for general-purpose use, but you should consider adding a constructor which takes
(int, Random) for testing purposes.@Override
public boolean hasNext() {
return ( unused.size() > 0 );
}I generally recommend favouring
isEmpty() over size() == 0. For many collections isEmpty() just calls size(), but for some it is much more efficient, so it's a good habit to always use isEmpty().int idx = random.nextInt(size);
int result = unused.get(idx);
unused.remove(idx);This is the biggest problem, although for your typical use case it might be relatively minor because it doesn't sound like this iterator is going to be a bottleneck.
Removing a random element from a list takes \$O(n)\$ time. But since you don't care about the order of the elements in the list, you could copy the last element to position
idx and then delete the last element in \$O(1)\$ time.Code Snippets
public NonRepeatingRandom(int max) {@Override
public boolean hasNext() {
return ( unused.size() > 0 );
}int idx = random.nextInt(size);
int result = unused.get(idx);
unused.remove(idx);Context
StackExchange Code Review Q#162035, answer score: 5
Revisions (0)
No revisions yet.