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;
      }
    }
}

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);
    }

}

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;
}