Condition arr[i] == arr[i+1]
would cause an exception during the last iteration step (when i=n-1
) with 100%
certainty because i+1
will refer to the index that doesn't exist. You need to get rid of it.
contains()
check on a List
is expensive, it has O(n) time complexity. Which results in the overall quadratic O(n^2) time complexity of your solution.
As @Scary Wombat has pointed out in the comment, you can use HashSet
to check whether the element was previously encountered or not. It would also eliminate the need for sorting the data and time complexity of the solution would be linear O(n).
In case if you have a requirement the result should be sorted, then it'll be more efficient to sort the resulting list (because it would contain fewer data than the given array).
public static List<Integer> duplicates(int[] arr, int n) {
Set<Integer> seen = new HashSet<>();
List<Integer> duplicates = new ArrayList<>();
for (int i = 0; i < n; i++) {
int next = arr[i];
if (!seen.add(next)) {
duplicates.add(next);
}
}
if (duplicates.isEmpty()) {
duplicates.add(-1);
}
// duplicates.sort(null); // uncomment this Line ONLY if results required to be in sorted order
return duplicates;
}
Sidenotes:
- Don't use concrete classes like
ArrayList
as a return type, type of variable, method parameter, etc. Leverage abstractions, write the code against interfaces like List
, Set
, etc. If coding platform wants you to return an ArrayList
, that unfortunate - leave the type as is, but keep in mind that it's not the way to go. See What does it mean to "program to an interface"?
- Avoid using C-style of array declaration
int arr[]
. Because it mixes the variable name and the type, which might look confusing. int[] arr
is preferable.