Checking an array for duplicate values

Question

This function checks an array for duplicated values. If it finds a duplicate value, it increments the variable name repeatedTime and then if repeatedTime is greater than 1 it calls a randomize function which produce different values in the array that was passed to the function. It repeats these steps until a duplicated value free array is produced and then notDuplicate becomes true and the function ends.

Are there any improvements that can be made to the function? I am thinking about making it into a recursive function. Is that feasible?

private void CheckForDuplicates(int[] selectedWordIds, List<Integer> answersTempId) {
 boolean notDuplicate = false;
 while(notDuplicate != true){
 int repeatedTime = 0;
 for(int x = 0; x < selectedWordIds.length ; x++)
 {
 for(int y =0; x < selectedWordIds.length;x++)
 {
 if(selectedWordIds[x] == selectedWordIds[y])
 {
 repeatedTime++;
 }
 }
 }
 if(repeatedTime > 1)
 {
 notDuplicate = false;
 RandomizeFunction(selectedWordIds, answersTempId);
 }
 else{
 notDuplicate = true;
 }
 }
}

• nattyddubbs: Why do you need answersTempId? Perhaps it plays a role in your Randomization method however that is not clear.

Answer 1

I suggest you use a Set when checking for duplicates:

Set<Integer> set = new HashSet<>();
for(int i : selectedWordIds) {
 if(!set.add(i)) {
 repeatedTime++;
 }
}

This is much simpler that what you have.

Here:

 if(repeatedTime > 1)
 {
 notDuplicate = false;
 RandomizeFunction(selectedWordIds, answersTempId);
 }

The assignment notDuplicate = false is not necessary because notDuplicate is already false.

About recursion: It is possible. With it, here is the final code:

private void CheckForDuplicates(int[] selectedWordIds,
 List<Integer> answersTempId) {
 int repeatedTime = 0;
 Set<Integer> set = new HashSet<>();
 for(int i : selectedWordIds) {
 if(!set.add(i)) {
 repeatedTime++;
 }
 }
 if (repeatedTime > 1) {
 RandomizeFunction(selectedWordIds, answersTempId);
 CheckForDuplicates(selectedWordIds, answersTempId);
 }
}

• Eric Stein: Recursion is possible but not called for. All you're doing is keeping memory allocated in the heap for longer than necessary.
• Christian Hujer: While I like recursion and use it frequently myself in Java, we need to keep in mind that Java is (Oracle, are you listening?!!!) not performing tail recursion optimization, i.e. every single recursive step of a recursion really allocates a new stack frame in Java.

Answer 2

• I'd expect that at the very first iteration (when both x and y are 0) the selectedWordIds[x] == selectedWordIds[y] expression would be true no matter what.

• Single Responsibility Principle dictates that CheckForDuplicates shall check for duplicates, no more no less. Randomization is not in its scope.

Answer 3

Leveraging both existing answers (@MannyMeng, @vnp).

Some notes:
0. In Java, methods start with a lowercase letter.
1. It's bad form for randomizeFunction() to mutate the array that's passed in. Better would be if it returned a new array instead, but that's out of scope for what you asked to be reviewed.

private boolean hasDuplicates(final int[] selectedWordIds) {
 final Set<Integer> set = new HashSet<>();
 for (final int i : selectedWordIds) {
 if (!set.add(i)) {
 return true;
 }
 }
 return false;
}
private int[] removeDuplicates(
 final int[] selectedWordIds, 
 final List<Integer> answersTempId) {
 final int[] wordIds = 
 Arrays.copyOf(selectedWordIds, selectedWordIds.length);
 while (hasDuplicates(wordIds)) {
 randomizeFunction(wordIds, answersTempId);
 // better would be non-final wordIds and
 // wordIds = randomizeFunction(wordIds, answersTempId);
 }
 return wordIds;
}