Best practices question on Seek and Destroy

Best practices question on Seek and Destroy
0

#1

The code below passes the tests:

    function destroyer(arr) {
      for (let i = 1; i < arguments.length; i++) {
        for (let j = 0; j < arr.length; j++) {
          if (arr[j] === arguments[i]) {
            arr.splice(j, 1);
            j--;
           }
         }
       }
    return arr;
    }

My question involves the use of j-- on the 6th line. I came across the problem that when I was iterating through the array, if I sliced out the second to last character, the last character was ignored in iteration. If the last character matched one of the arguments, it wasn’t deleted, thus failing the tests.

I chose to try j-- rather than rewrite the for loop as a while loop with 2 if blocks and manual iteration of the j variable, but the solution is nagging at me. It feels like a hack rather than a true solution. I guess my question is, does this solution fall within the boundaries of JS best practices, or is there another way to come to the same solution that works better?


#2

I’ve done the same thing too. Is it a little hinky? Probably. Is it dangerous? Maybe a little. Is it terrible? Probably not.

What if I’d written it this way?

let j=0;
while (j < arr.length) {
   if (arr[j] === arguments[i]) {
      arr.splice(j, 1);
      j--;
  }
  j++;
}

Would that seem wrong? We think of the for loop as some sacrosanct temple of iteration, but there is no reason why you can’t manipulate it. In some languages you can’t do this, but maybe I’m jaded because I come from a C background where you can get away with murder.

But there is a caveat. You must be careful! It is very easy to end up with an infinite loop if you make a mistake. You must make sure your logic is sound and you have considered all possible conditions.


#3

Your intuition is right, but “better” is a concept that should be defined in some sort of context. Should the algorithm run faster? You could save one operation for each length property lookup by caching the value.

for ( let i = 1, length = arguments.length; i < length; i++) {
    // ...
}

Each property lookup is a constant time operation, so you’re saving 1(arguments.length + arr.length) number of operations each time the function is run. This could be significant if either array is yyyyuuuuuuggggge, but for most programs you’ll ever write it won’t amount to much. However, your code changes the length of arr during runtime, so it’s not really an option here.

If you take a look at the advanced solution in the wiki, you’ll find some programming constructs you may or may not have seen yet. These constructs are actually slower in runtime than writing out for loops, but they are generally preferred because they give us far more readable code, and readability is where we ought to put our efforts as programmers. Nothing in your computer cares whether you decrement j or where you do it. Your program will be executed, and as @ksjazzguitar notes, this will include any mistakes you’ve made. Your code, as-is, is fine, but imagine that there’s a lot more inside that if statement, or that there’s many more branches to the logic. How easy will it be to keep track of where j is being decremented? How easy will it be for another programmer to come in and understand your work? The harder either of these things are, the easier it is to write a bug. For this reason, I think a while loop is better, but you could still use a for loop if you understand the syntax.

for (let j = 0; j < arr.length;) {  //remove j++
    if (arr[j] === arguments[i]) 
        arr.splice(j, 1);
     } else {
        j++; // It's now clear when and why j will increment
     }
}

I think we can still do better, and keep in mind that I’m not going to show you the advanced solution. The use of slice means we’re modifying the array that’s passed in, which complicates things. What if we obviated all the logic involved in changing the iterator variable by creating a new array in the function?

function destroyer(arr) {
    var inputs = Array.prototype.slice.call(arguments).slice(1);
    var newArr = [];
    for(var i = 0, length = arr.length; i < length; i++) {
      if(inputs.indexOf(arr[i]) === -1) {
          newArr.push(arr[i]);
      }
    }
    return newArr;
}

Now, this might run faster than your code, and I think it will use a little more memory as a trade-off, but IMO the only real way my code is better is simply that it’s more readable.