-2
\$\begingroup\$
  public static Integer[] eliminateDuplicate(int[] a) {
        if (a == null) {
            throw new NullPointerException();
        }
        List<Integer> list = new ArrayList<Integer>();

        for (int i = 0; i < a.length; i++) {
            // include last element by default, if not the last element, do the check for duplication.
            if (i == a.length - 1 || a[i] != a[i + 1]) {
                list.add(a[i]);
            }
        }
        return list.toArray(new Integer[0]);
    }
\$\endgroup\$
2
  • 1
    \$\begingroup\$ It would help if you added some comments about your intent and any problems you encountered or things you are unsure of. Code is not, in itself, documentation. \$\endgroup\$
    – itsbruce
    Aug 31, 2013 at 21:15
  • \$\begingroup\$ Please add a comment detailing what you want improvement on in this code. \$\endgroup\$
    – aydjay
    May 14, 2015 at 13:14

2 Answers 2

2
\$\begingroup\$

Your function doesn't handle duplicates in the entire array, but only in consecutive entries. A better name might be collapseConsecutive.

The input is an int[], but the return type is Integer[]. Consistency would be nice.

You don't need to throw NullPointerException explicitly. If a is null, then a NullPointerException will automatically be thrown when it tries to do a.length. If you really want to explicitly validate your arguments, you could throw IllegalArgumentException instead.

A micro-optimization would be to change to

if (i == 0 || a[i] != a[i - 1]) {
    list.add(a[i]);
}

Comparing against 0 is simpler than comparing against a.length - 1 since the JVM (and CPUs) has an ifeq opcode for doing just that. To compare against a.length - 1, the bytecode would have to load a.length - 1 into a register before comparing — and that is assuming that the compiler is smart enough to compute a.length - 1 just once.

\$\endgroup\$
1
  • \$\begingroup\$ See, JD? It's not just me not being sure of your intent. \$\endgroup\$
    – itsbruce
    Aug 31, 2013 at 21:39
2
\$\begingroup\$

Minor point - don't use an old c-style for loop to iterate through an array unless you absolutely have to (and you don't have to here). Please, this is 2013 not 2003 - generics have been here for a decade. You can use foreach:

if a.length > 0 {
  int lastValue = a[0];
  list.add(lastValue);
  for (int i: a) {  
    if (i != lastValue) {
      lastValue = i;
      list.add(i);
    }
  } 
}

Using old-style c-loops always opens a possibility of a stupid error. Since you can cache the last value you inserted in the array, you don't have to use the old-style loop. My loop also has just one check, and that check is cheaper to do than yours (or 200_success's) because it doesn't need to do the array look-up.

Note that my solution inserts the first value without checking, while yours does this for the final one. Not only is this more logical (I think), it eliminated a check from the logic.

Your method is called eliminateDuplicates but it will only do that if the list has already been sorted. If that was not done, it will only eleminiate sequential duplicates. That is, it will turn (3, 1, 2, 3, 3, 4, 3) into (3,1,2,3,4,3).

Is that what you want? The fact that you always keep the last member implies this, but how can we be sure? If it is so, either document the intent or maybe name the function more clearly (e.g. eliminateAdjacentDuplicates).

If you want to eliminate all duplicates, then you could start by sorting the array. But since you are using the collections api, why not use a set?

Set<Integer> intSet = new HashSet<Integer>(Arrays.asList(a));

Then you just need to turn the set back into an array.

I may have gone on at unnecessary length, but that's because you didn't properly document your intent. Please, if you learn nothing else, learn this.

\$\endgroup\$
1
  • \$\begingroup\$ Since you accepted 200_success's answer, I guess you wanted to eliminate adjacent duplicates ;) But I've left my original comments in, despite having edited my code for clarity. \$\endgroup\$
    – itsbruce
    Aug 31, 2013 at 22:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.