current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote -2 down vote favorite 
  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]);
    }
share|improve this question
1  
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. –  itsbruce Aug 31 '13 at 21:15
    
Please add a comment detailing what you want improvement on in this code. –  d347hm4n May 14 at 13:14

2 Answers 2

up vote 1 down vote accepted

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.

share|improve this answer
    
See, JD? It's not just me not being sure of your intent. –  itsbruce Aug 31 '13 at 21:39
up vote 2 down vote

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.

share|improve this answer
    
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. –  itsbruce Aug 31 '13 at 22:32

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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