3
\$\begingroup\$

In this question, the suggested solution for smoosh() is as shown below,

public static void smoosh(int[] a) {
    int originalPos = 0;
    int targetPos = 0;
    while (originalPos < a.length) {
        // Copy (and remember) one element to the correct position:
        int currentElement = a[targetPos++] = a[originalPos++];
        // Advance originalPos until a different element is found:
        while (originalPos < a.length && a[originalPos] == currentElement) {
            originalPos++;
        }
    }
    // Fill remaining elements:
    while (targetPos < a.length) {
        a[targetPos++] = -1;
    }
}

but, I felt that this well tested new solution shown below looks more readable than shown above,

  public static void smoosh(int[] a) {
    int saveOriginalPosition = 1;
    int targetPosition = 0, originalPosition = 0;
    while(targetPosition < a.length){
        if(saveOriginalPosition < a.length){
            for(originalPosition = saveOriginalPosition;originalPosition < a.length;originalPosition++)
                if(a[originalPosition] != a[targetPosition])
                {
                    a[++targetPosition] = a[originalPosition];
                    break;
                }
            saveOriginalPosition = originalPosition;
        }else{
            break;
        }
    }
    if(originalPosition == a.length){
        for(int setRemaining = targetPosition+1; setRemaining  < a.length; setRemaining++)
            a[setRemaining] = -1;
    }

 }

because point mentioned by 200_success, i.e., "However, being able to recognize the structure of the loop just by looking at the loop header gives it an advantage in readability." is considered in this new solution, whereas the inner while loop of the suggested solution does not look readable.

Please let me know your inputs on the changes.

\$\endgroup\$
1
  • \$\begingroup\$ i did not prefer writing while((targetPosition < a.length) && (saveOriginalPosition < a.length)){ for the same reason and instead left a break with if{} block. \$\endgroup\$ Commented Oct 28, 2014 at 2:44

1 Answer 1

5
\$\begingroup\$

I think there are issues with both implementaitons, though I do find the first more readable.

The main problem is that neither makes it immediately clear that we are assigning a new value to each element of the input array. I would expect an implementation to look something like this:

public static void smoosh(int[] input) {
  for (int i = 0; i < input.length; i++) {
    input[i] = ...
  }
}

This makes it clear to the reader that each element of the input array will be assigned to.

Now we can keep track of where the next element for our array is coming from:

public static void smoosh(int[] input) {
  int j = 0;
  for (int i = 0; i < input.length; i++) {
    input[i] = j < input.length ? input[j] : -1;
    j++;
    while (j < input.length && input[i] == input[j]) {
      j++;
    }
  }
}

It's getting hard to read again, so let's split the logic for j into its own method:

public static void smoosh(int[] input) {
  int j = 0;
  for (int i = 0; i < input.length; i++) {
    input[i] = j < input.length ? input[j] : -1;
    j = getNextDifferentIndex(input, j);
  }
}

/** Get the smallest integer j such that j > i and input[i] != input[j].
 * If no such element exists, return an integer greater than or equal to
 * input.length. */
private static int getNextDifferentIndex(int[] input, int i) {
  int j = i + 1;
  while (j < input.length && input[i] == input[j]) {
    j++;
  }
  return j;
}

Or if you want to stick to the 14-line limit mentioned in the previous question,

public static void smoosh(int[] input) {
  for (int i = 0, j = 0; i < input.length; i++, j = getNextDifferentIndex(input, j)) {
    input[i] = j < input.length ? input[j] : -1;
  }
}

private static int getNextDifferentIndex(int[] input, int i) {
  int j = i + 1;
  while (j < input.length && input[i] == input[j]) {
    j++;
  }
  return j;
}
\$\endgroup\$
1
  • \$\begingroup\$ For your point:neither makes it immediately clear that we are assigning a new value to each element of the input array Do you mean: a[++targetPosition] = a[originalPosition]; does not convey this msg? \$\endgroup\$ Commented Oct 30, 2014 at 9:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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