Just want to check on my understanding of this randomising function

The code below works fine, though I’ve no doubt there could be more efficient ways of achieving it. The code itself was provided in an undocumented form, so I’ve added my own comments into it just to be sure I understand what’s going on, particularly on the for loop that’s randomising the array.

I can seem what that particular for loop is doing, but am I right in thinking that later passes through to loop could move numbers that have already been moved out of sequence, one, or possibly more times, depending on what random number is being used at the time?

function init()
{
  // Create variables
  var panel = document.getElementById( "panel" );
  var i, rand, temp, str, nums = [];

  // Create an array containing the numbers 1 to 49
  for ( i = 1; i < 50; i++ )
  {
    nums[ i ] = i;
  }

  // Randomise the contents of the array 'nums'
  for ( i = 1; i < 50; i++ )
  {
    // Create a random number with a value between 1 and 49 and assign it to the variable 'rand'
    rand = Math.ceil( Math.random() * 49 );
    // Assign the value of nums[i] to a variable called temp
    temp = nums[i];
    // Assign the value of nums[rand] to nums[i]
    nums[i] = nums[rand];
    // Assign the value of the variable 'temp' to nums[rand]
    nums[rand] = temp;
  }

  //Output the first 6 numbers in the array 'nums' to the element with the ID of 'panel'
  str = "Your Six Lucky Numbers are: ";
  for ( i = 1; i < 7; i++ )
  {
    str += nums[ i ];
    if ( i !== 6 ) 
    {
      str += " - ";
    }
  }
  panel.innerHTML = str;
}
document.addEventListener( "DOMContentLoaded", init, false );

Last thought, am I right in thinking that, because the first two for loops are independently counting from 1 to 49, that there is a potential for the function to break were they to be given different values, or does it just affect how much of the array is used, or the number of times it is iterated over to randomise it?

1 Like

Hey Chris,

As I read this, I was reminded of a really cool tool that @Jeff_Mott shared recently, that allows you to visualize the execution of JavaScript code.

I put in a modified version of the code - just the section with the first two loops, and an array of only 10 numbers, to make it easier to visualize. This link will take you to the point in execution just before the second loop starts, and you can step forward incrementally and watch what happens to the array and the temporary variables.

Yeah, if the first loop runs more times than the second, you’d end up with an array that’s only partially randomized. If the second loop runs more, you’ll end up with an array with some undefined values (e.g when the value of i is greater than the number of elements in the nums array + 1, doing nums[i] will return undefined).

2 Likes

Thanks @fretburner, that tool looks like exactly the kind of thing I need to help keep a track of what’s going on in my JS. I’ll be taking a close look at that.

I find that with much tutorial material, the conceptualisation of what the code is doing is kind of glossed over in favour of getting an end result that works. With that tool, you do kind of get a better sense of how what the code is doing is built up.

Seems a rather lot of code to do something really simple.

If I understand what your code is supposed to be doing (which isn’t obvious) then the following does the same thing.

var populateArray = function(n) {
return Object.keys(Object(0+Array(n)));
};
var shuffle = function(c) {
   var r=[];
   while (c.length) r.push(c.splice(Math.random() * c.length, 1)[0]);
   return r;
}; 
var nums = populateArray(50); // create an array containing 0-49
nums.shift(); // drop the 0 off the front of the array
nums = shuffle(nums); // shuffle the array
panel.innerHTML = nums.slice(43).join('-'); // output the last 6 entries with hyphens between

Having it obvious what the code is doing is always important, In the above code I have put the not so obvious code in functions where the function name hopefully makes it more obvious what the code in the function does.

It is, but that’s what happens when you’re learning from pre-formed code examples from what is, admittedly, a rather basic JS tutorial book, and the associated code download files. I guess I’ve learned enough to recognise when things can be improved, but I’ve not necessarily got enough understanding to be able tackle the “how” just yet.

Just for clarity’s sake, I dropped the whole thing into a Codepen so it can be seen in action [sic]

Looking at the original code posted that presumably came from the book it isn’t obvious to me how the main loop works.

Sometimes these books take something really simple to do in JavaScript and introduce it too early where it then becomes really complicated to do because the one or two commands needed to do it easily haven’t been taught yet.

…Is this the loop we’re talking about…?

  // Randomise the contents of the array 'nums'
  for ( i = 1; i < 50; i++ )
  {
    // Create a random number with a value between 1 and 49 and assign it to the variable 'rand'
    rand = Math.ceil( Math.random() * 49 );
    // Assign the value of nums[i] to a variable called temp
    temp = nums[i];
    // Assign the value of nums[rand] to nums[i]
    nums[i] = nums[rand];
    // Assign the value of the variable 'temp' to nums[rand]
    nums[rand] = temp;
  }

Yes. If it were not for the comments it would not be obvious just from a quick glance as to what that code is actually trying to do. You have to actually spend time examining the code in order to see what it is doing.

It becomes obvious once you do examine the loop in sufficient detail that there is actually a bug in that processing. Math.random() returns a value greater than or equal to 0 and less than one. Multiplying by 49 and rounding up to an integer leaves the last position in the array slightly less likely to be selected than the others and with a very slight chance of a non-existent zero entry being referenced.

Code that is less obvious as to what it is doing at first glance is more likely to contain bugs as this loop does. Where the code is easier to read in the first place a bug such as the one this code contains would be easier to spot.

You’re right about OP’s indexing bug, and you picked the better algorithm, but if the quality we’re comparing is readability and “obviousness” of the code, then I think the OP’s original is better than what you posted.

You try to pack too much into a single line. I think that’s bad.
You still insist on using single-letter variable names. I think that’s bad.
And you rely on weird, unintuitive, and esoteric behavior. I think that’s bad.

[quote=“Jeff_Mott, post:10, topic:218020”]
You try to pack too much into a single line. I think that’s bad.
You still insist on using single-letter variable names. I think that’s bad.[/quote]

That’s a result of my grabbing functions out of my collection where the code has been partly compressed.

As the functions are only a couple of lines long I don’t think that having them like that is all that harmful since you don’t actually need to work with the code inside the function, you just need to call it.

As it is one line of code inside a function the exact code used to produce a given result is not all that important.

I happen to know exactly how this version works

var populateArray = function(n) {
return Object.keys(Object(0+Array(n)));
};

the following version produces the same result and is only slightly longer (as well as being a lot more obvious what it is doing - provided you know about using Array.apply(null Array(n)) to generate dense arrays).

var populateArray = function(n) {
return Array.apply(null, Array(n)).map(function (z,i) {return i});
} 

Is the following variation of the code easier to follow?

I’ve tried to make the shuffle easier to follow too, by using the bog-standard fisher-yates shuffle

var createIndexedArray = function (start, end) {
        var indexedArray = [],
            i;
        for (i = 0; i <= (end - start); i += 1) {
            indexedArray[i] = start + i;
        }
        return indexedArray;
    },
    shuffle = function fisherYatesShuffle(array) {
        // See: https://en.wikipedia.org/wiki/Fisher-Yates_shuffle
        var i = 0,
            j = 0,
            swap = function(array, i, j) {
                var temp = array[i];
                array[i] = array[j];
                array[j] = temp;
            };
        for (i = array.length - 1; i > 0; i -= 1) {
            j = Math.floor(Math.random() * (i + 1));
            swap(array, i, j);
        }
        return array;
    },
    numericSort = function numeric(a, b) {
        return a > b;
    },
    lottoNumbers = function () {
        var nums = createIndexedArray(1, 49);
        nums = shuffle(nums);
        return nums.splice(0, 6).sort(numericSort);
    };

panel.innerHTML = lottoNumbers().join('-');
1 Like

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.