HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Iterator producing unique random numbers in a specified range

Submitted by: @import:stackexchange-codereview··
0
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).

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:

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.