CODEWITHSUNDEEP
javascriptshuffle
BeerBeard
BeerBeard

Reputation: 93

What is wrong with my shuffling program?

I wrote a shuffling program below and ran it through "Will It Shuffle?". The results appear to show that it's working in the console; it's shuffling the array. But the website shows me an all red box, making me think something is wrong with my code, but I don't see it.

function shuffle (array) {
    var arr = [],
        length = array.length,
        el;

    while (length > 0) {
      var randomEl = Math.floor(Math.random() * (length - 0) - 0);
      if (length > 1) {
        el = array.splice(randomEl,1);
      } else {
        el = array.splice(0,1);
      }
        arr.push(el[0]);
        length -= 1;
    }

  return arr;
}

Upvotes: 0

Views: 46

Answers (3)

Matt Broatch
Matt Broatch

Reputation: 731

What you are doing is creating a new array with the elements of the original shuffled around.

However, if you go back and look at the array that you passed in, you'll notice it has not been shuffled, but rather emptied. Apparently, this is not what "Will it Shuffle?" asks you to do.

splice() and push() both always mutate the array you call those methods on.

To answer your question about .push(...arr), an elipses in javascript is a feature that arrived with the latest version, EcmaScript 2015. It is the "spread operator".

When you call a function with a "spread" array, it's like calling the function with the contents of the array as separate arguments. For instance,

array.push(...[1,2,3])

is the same as calling

array.push(1,2,3)

push() can add an arbitrary number of comma-separated arguments to an array. So, after emptying the array argument with your looped splices, you could push the contents of the newly-created arr to the empty array using the spread operator.

Upvotes: 1

epascarello
epascarello

Reputation: 207501

They alter the array, you do not alter the array.

You need to alter the original array, not return a new array.

function shuffle (array) {
    var arr = [],
        length = array.length,
        el;

    while (length > 0) {
      var randomEl = Math.floor(Math.random() * (length - 0) - 0);
      if (length > 1) {
        el = array.splice(randomEl,1);
      } else {
        el = array.splice(0,1);
      }
        arr.push(el[0]);
        length -= 1;
    }

  //replace array with the new items 
  //it is like using concat, but does not produce a new array, 
  //just appends it to the original which has zero items in it.
  Array.prototype.push.apply(array, arr);  



}

Upvotes: 1

Oriol
Oriol

Reputation: 288100

That page ignores the returned value of the function, because it expects an in-place sort.

If you add this at the end of your code, it works as expected:

array.push(...arr);

You can also do it in-place directly:

function shuffle (array) {
  var length = array.length;
  while (length) {
    var randomEl = Math.floor(Math.random() * length);
    var el = array.splice(randomEl, 1);
    array.push(el[0]);
    --length;
  }
}

Upvotes: 1

Related Questions