HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Comparing two implementations of smoosh()

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
smooshimplementationstwocomparing

Problem

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.

Solution

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

Code Snippets

public static void smoosh(int[] input) {
  for (int i = 0; i < input.length; i++) {
    input[i] = ...
  }
}
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++;
    }
  }
}
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;
}
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;
}

Context

StackExchange Code Review Q#68136, answer score: 5

Revisions (0)

No revisions yet.