javascript function don't pass cases

Question

I have no idea why my function doesn't work in some cases:

function moveZeros(elem) {
  var count = 0;
  var a=elem;
  for (var i=0; i< elem.length; i++){
    if (elem[i]===0) {
    elem.splice(i,1);
    count++;
    }
  } 
  while (count>0) { 
    elem.push(0); 
    count--;
    }
    return  elem;
}

In moveZeros([1,2,0,1,0,1,0,3,0,1]) all good, but if case is:

moveZeros([9,0.0,0,9,1,2,0,1,0,1,0.0,3,0,1,9,0,0,0,0,9])

it returns

[9,0,9,1,2,1,1,3,1,9,0,0,9,0,0,0,0,0,0,0]

I case :

moveZeros(["a",0,0,"b","c","d",0,1,0,1,0,3,0,1,9,0,0,0,0,9]) it returns: ["a",0,"b","c","d",1,1,3,1,9,0,0,9,0,0,0,0,0,0,0]

Why not all zeros goes to the end ?

Mostly a guess since I haven't debugged this directly (have you?), but should you be adjusting the value of `i` in your loop when you modify the array? — David, Aug 15 '17 at 15:54
Related: [Looping through array and removing items, without breaking for loop](https://stackoverflow.com/questions/9882284/looping-through-array-and-removing-items-without-breaking-for-loop) — Jonathan Lonowski, Aug 15 '17 at 15:56

adeneo · Answer 1 · 2017-08-15T17:22:20.270

3

Other ways to do this include filtering out the zeros and using Array.fill to repad with zeros at the end

function moveZeros(elem) {
  var f=elem.filter(x=>x!==0);
  return f.fill(0, f.length, f.length=elem.length);
}

console.log( moveZeros([9,0.0,0,9,1,2,0,1,0,1,0.0,3,0,1,9,0,0,0,0,9]) );

edited Aug 15 '17 at 17:22

answered Aug 15 '17 at 16:24

adeneo

312,895
29
395
388

Nice alternative approach. I would probably change this to initialize `c` to `elem.length` at the start, eliminate it from the `filter` callback, and then just fill from `f.length` to `f.length = c`. (I'd also rename `c` to `n` or `len`, since it would no longer be a count.) – Ted Hopp Aug 15 '17 at 16:45
But `c` is in fact a count, it counts the number of zeros removed to add them back – adeneo Aug 15 '17 at 16:48
It would no longer be a count after my suggested refactoring. I'm suggesting that counting be eliminated and instead use the original array length to determine how many zeros to add back. It seems like less work that way. – Ted Hopp Aug 15 '17 at 16:49
I see, but I'd still have to use `f.length+=c` as that resets the length of the array as well, otherwise I can't fill it. But you have a point, I could shorten it by removing the count and the ternary ... updating – adeneo Aug 15 '17 at 16:52
Just use `f.length = c` instead of `f.length += c` (once the semantics of `c` changes from a count to the original length). – Ted Hopp Aug 15 '17 at 16:53
So I see! It's also worth mentioning that this is no longer equivalent to OP's (apparent) original intent, because it leaves the argument unmodified. – Ted Hopp Aug 15 '17 at 16:58
It does not change the original array, but returns a new one, so yes, it's a bit different, but the OP is still returning the array, indicating that this would work just as well. We could go all ninja and make it a one-liner -> https://jsfiddle.net/tf0w6uLd/3/ – adeneo Aug 15 '17 at 16:59

score 2 · Accepted Answer · answered Aug 15 '17 at 15:56

2

When you're removing elements from an array while iterating over it, you need to go from the end of the list to the beginning. This is because when you .splice the element out, i is no longer pointing to the index it was previously pointing to, so it'll skip over some indices. Try this instead.

function moveZeros(elem) {
  var count = 0;
  var a=elem;
  for (var i = elem.length - 1; i >= 0; i--){
    if (elem[i]===0) {
    elem.splice(i,1);
    count++;
    }
  } 
  while (count>0) { 
    elem.push(0); 
    count--;
    }
    return  elem;
}

answered Aug 15 '17 at 15:56

PunDefeated

566
5
18

`for (var i=elem.length; i--;){` seems more intuitive – adeneo Aug 15 '17 at 15:59
Isn't that missing the condition statement? Or am I just confused? – PunDefeated Aug 15 '17 at 16:04
I meant instead of `for (var i = elem.length - 1; i >= 0; i--){` – adeneo Aug 15 '17 at 16:04
If you skip the ```i >= 0;``` what happens? I just haven't seen for loops written without the conditional statement. – PunDefeated Aug 15 '17 at 16:16
Then it'll constantly loop, and you'll need to use any of the following to get out of the loop: `break` `continue label` `throw expression` or `return`. See the documentation on empty statements: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/Empty – Pluto Aug 15 '17 at 16:24
1

The condition is not missing; it's `i--`, which evaluates to `false` when `i` is 0 (and becomes -1). The condition serves double duty as also decrementing `i` each time through the loop (including just before the first execution of the loop body, which is why @adeneo initializes `i` to `elem.length` instead of `elem.length-1`). It's logically equivalent to your code. (But I'd hardly call it more intuitive.) – Ted Hopp Aug 15 '17 at 16:26
@TedHopp - that is correct, and I think it's more intuitive that way, but maybe that's because I'm used to seeing reversed loops done that way. It's the same thing, I just find the shorter version easier to read. – adeneo Aug 15 '17 at 16:56

score 0 · Answer 3 · answered Aug 15 '17 at 15:58

Your problem is that when you remove an element the next item processed is the original location plus one i.e. with the array [1,0,0,1] you will process the 1 (index 0) then the first 0(index 1) and then the last 1(index 2); The second 0 was at index 2 when you started but was skipped because removing the first 0 changed it's index to 1.

To process the array correctly you want to subtract 1 from i whenever you find a match -

function moveZeros(elem) {
  var count = 0;
  var a=elem;
  for (var i=0; i< elem.length; i++){
    if (elem[i]===0) {
        elem.splice(i,1);
        count++;
        i--;
    }
  } 
  while (count>0) { 
    elem.push(0); 
    count--;
    }
    return  elem;
}

marvel308 · Answer 4 · 2017-08-15T16:18:31.000

0

Why don't you do something like

function moveZeros(arr){
    let idx =0;
    for(let i = 0; i<arr.length; i++){
        if(arr[i] == 0){
            idx++;
        }
        else{
            arr[i-idx] = arr[i];
        }
    }
    for(let i = arr.length-idx; i< arr.length; i++){
     arr[i] = 0;
    }
    console.log(arr);
}

moveZeros([1, 2, 3, 0, 0, 4, 5, 0, 0, 6]);
moveZeros([1,2,0,1,0,1,0,3,0,1]);
moveZeros([9,0.0,0,9,1,2,0,1,0,1,0.0,3,0,1,9,0,0,0,0,9])

it does it in one pass without having to use splice() which is costly

edited Aug 15 '17 at 16:18

answered Aug 15 '17 at 16:09

marvel308

10,288
1
21
32

I meant without using splice which is costly, and in one pass – marvel308 Aug 15 '17 at 16:18
You can replace the last loop with a call to `arr.fill(0, arr.length-idx)`. – Ted Hopp Aug 15 '17 at 16:46

javascript function don't pass cases

4 Answers4