Your choice of algorithm is nearly perfect as long as the number of elements selected is significantly smaller than the size of the wordlist (Otherwise, the number of duplicates starts to dominate the runtime, and you would be better off doing a partial sort on a copy of the array, as bowmore suggests).
However, like corsika, I'd use a Set
for efficient duplicate testing.
You should create only a single instance of Random
, and use that for all iterations of the loop, rather than creating a new instance of Random
for every iteration. The reason is that Random
's default constructor seeds its random number generator by the current system time, which on many platforms is updated infrequently enough that it will be the same for all iterations of the loop, causing you to draw the same random number many times.
Next, advancing and rewinding the loop variable in different statements makes it needlessly hard to understand the loop. I'd find it clearer to only modify the variable in one place. Same for removing and readding the same word to the result list.
As a matter of style, it is redundant to compare a boolean with true. That is, whenever you find yourself writing expr == true
, you might as well simply write expr
, but it has exactly the same meaning.
While we are on the subject of redundant code, type parameters provided to a constructor can usually be inferred by the compiler, so there is no need to specify them.
Finally, I'd probably move sampling to a new method, so I can give that method a meaningful name.
Therefore, I'd do it something like this:
Set<String> sampleWithoutReplacement(List<String> words, int count) {
if (count > 0.8 * words.size()) {
// fall back on bowmore's solution or throw an exception
}
Random random = new Random(); // might wish to move this to a field instead (but keep thread safety in mind)
Set<String> results = new HashSet<>();
while (results.size() < count) {
results.add(words.get(random.nextInt()));
}
return results;
}