Simple for loop does not seem work properly

Question

I'm having a trouble to get an array with numbers from 1 to 16 randomly without repetition. I made num array to put numbers from createNum function.

The createNum function has a for loop which gets numbers from 1 to 16 until if statement push 16 numbers into num array.

At the end, I run createNum() and display numbers on the web. While I'm making this code it sometimes worked but now it's not working.

can somebody point out where I made mistakes?

let num = [];

function createNum () {
    for (i = 0; i <= 15;) {
        let numGen = Math.floor(Math.random() * 15) + 1;
        if (!num.includes(numGen)) {
            num.push(numGen);
            i++;
        };
    };
}
console.log(createNum());
document.getElementById("selectedNumbersShownHere").innerHTML = num;
console.log(num);

Thank you everybody, this problem was fixed! I understood there're many ways to fix the problem. thank you! — Kay. T, Dec 25 '19 at 15:13

Mister Jojo · Answer 1 · 2019-12-25T07:25:56.867

Your loop seems never finish because the probability to get the last value is very low, and never can happen in a short time.

Plus :
your formula is wrong : let numGen = Math.floor(Math.random() * 15) + 1;
and should be................: let numGen = Math.floor(Math.random() * 16) + 1; value 16

see => Generating random whole numbers in JavaScript in a specific range?

do this:

function createNum()
  {
  let num =[], lastOne =136; // 136 = 1 +2 +3 + ... +16
  for (;;)
    {
    let numGen = Math.floor(Math.random() *16) +1;
    if (!num.includes(numGen))
      {
      lastOne -= numGen;
      if (num.push(numGen)>14) break;
      }
    }
  num.push(lastOne); // add the missing one (optimizing)
  return num;
  }

let unOrdered_16_vals = createNum();

/*
document.getElementById("selectedNumbersShownHere").textContent = unOrdered_16_vals.join('');
*/

console.log( JSON.stringify( unOrdered_16_vals ), 'length=', unOrdered_16_vals.length  );
console.log(  'in order = ', JSON.stringify( unOrdered_16_vals.sort((a,b)=>a-b) ) );

remark : The push() method adds one or more elements to the end of an array and returns the new length of the array.

score 2 · Accepted Answer · answered Dec 25 '19 at 05:51

This is because Math.random() never returns 1, so at the end Math.floor(Math.random() * 15) will returns maximum 14 and adding it to 1 will get you maximum 15. Use Math.ceil instead of Math.floor i.e

let num = [];

function createNum () {
    for (i = 0; i <=15;) {
        let numGen = Math.ceil(Math.random() * 16);
        console.log(numGen)
        if (!num.includes(numGen)) {
            num.push(numGen);
             i++;
        };
    };
}
console.log(createNum());
document.getElementById("selectedNumbersShownHere").innerHTML = num;
console.log(num);

Hope it helps!

Thank you for your answer. I just knew what ceil is and think your answer fix the problem very well. thanks! — Kay. T, Dec 25 '19 at 15:08

score 2 · Answer 3 · answered Dec 25 '19 at 06:07

2

for (i = 0; i <= 15;) generates 16 numbers, but Math.floor(Math.random() * 15) + 1 only have 15 possible values (1~15).

A shuffle function is recommended. Your function would be slow if you generate a large size shuffled array.

How can I shuffle an array?

answered Dec 25 '19 at 06:07

Xixiaxixi

172
10

Thank you for the recommendation. I'll learn shuffle function you proposed. – Kay. T Dec 25 '19 at 15:06

Robby Cornelissen · Answer 4 · 2019-12-25T15:15:16.967

The problem in your code is that your looking for 16 distinct numbers out of 15 possible values.

The reason for this is that Math.floor(Math.random() * 15) + 1; will only return values between 1 and 15, but your loop will run until you have 16 unique values, so you enter into an infinite loop.

What you're basically trying to achieve is randomly shuffle an array with values from 1 to 16.

One common solution with good performance (O(n)) is the so-called Fisher-Yates shuffle. Here is code that addresses your requirements based on an implementation by Mike Bostock:

function shuffle(array) {
  let m = array.length, t, i;

  while (m) {
    i = Math.floor(Math.random() * m--);
    t = array[m];
    
    array[m] = array[i];
    array[i] = t;
  }

  return array;
}

// create array with values from 1 to 16
const array = Array(16).fill().map((_, i) => i + 1);

// shuffle
const shuffled = shuffle(array);

console.log(shuffled);

Compared to your approach and the approach of other answers to this question, the code above will only make 15 calls to the random number generator, while the others will make anywhere between 16 and an infinite number of calls^⁽¹⁾.

^{⁽¹⁾ In probability theory, this is called the coupon collector's problem. For a value n of 16, on average 54 calls would have to be made to collect all 16 values.}

Thank you for your answer. Let me have some time to understand your code. Thanks! — Kay. T, Dec 25 '19 at 15:11

Selaka Nanayakkara · Answer 5 · 2019-12-25T05:44:45.117

1

Try like this :

    let num = [];

function createNum () {
    for (i = 0; num.length <= 17; i++) {
        let numGen = Math.floor(Math.random() * 16) + 1;
        if (!num.includes(numGen)) {
            num.push(numGen);
        };
    };
}
console.log(createNum());
document.getElementById("selectedNumbersShownHere").innerHTML = num;
console.log(num);

Please find the working demo here

edited Dec 25 '19 at 05:44

answered Dec 25 '19 at 05:26

Selaka Nanayakkara

3,296
1
22
42

thanks, SelakaN. It works, but I need to get 16 numbers (I want to let the code run till it gets 16 different numbers). – Kay. T Dec 25 '19 at 05:28
Can you mark it as correct answer an upvote I'll upadte the answer accordingly – Selaka Nanayakkara Dec 25 '19 at 05:30
I can not yet. because it's not yet solving my question. – Kay. T Dec 25 '19 at 05:36
You're asking for upvotes and acceptance, and then you'll provide the answer? – Robby Cornelissen Dec 25 '19 at 05:37
yes, but if I use your update it doesn't make 16 numbers. your update now only runs the code 16 times. That's where I'm having headaches at... – Kay. T Dec 25 '19 at 05:40
Op has stated 'SelakaN. It works' Why not ask for that. I have so many bad experiences where OP's never marked it as correct leaving for good solutions. – Selaka Nanayakkara Dec 25 '19 at 05:43
Updated my answer please check now @Kay.T – Selaka Nanayakkara Dec 25 '19 at 05:45
Hope it helps kindly mark it as correct if it works. THanks in advance – Selaka Nanayakkara Dec 25 '19 at 05:48

score 0 · Answer 6 · answered Dec 25 '19 at 05:42

This is an infinite loop error. Because your loop variable "i" is always less than or equal to 15. and your i++ is inside the if statement. You can achieve it in multiple ways. Below is one sample.

let num = [];

function createNum () {
    for (i = 0; i <= 15;) {
        let numGen = Math.floor(Math.random() * 15) + 1;
        if (!num.includes(numGen)) {
            num.push(numGen);
        };
         i++;
    };
}
console.log(createNum());
document.getElementById("selectedNumbersShownHere").innerHTML = num;
console.log(num);

score 0 · Answer 7 · answered Dec 25 '19 at 17:21

sorry, I'm "passionate" about this question, and I can't resist presenting a second answer, which is IMHO: the best!

function createNum ()
  {
  let num = []
  for (let len=0;len<16;)
    {
    let numGen = Math.ceil(Math.random() * 16)
    if (!num.includes(numGen))
      { len = num.push(numGen) }
    }
  return num
  }

let unOrdered = createNum();

console.log( JSON.stringify( unOrdered ) );

/*
document.getElementById("selectedNumbersShownHere").textContent = unOrdered_16_vals.join('');
*/

Simple for loop does not seem work properly

7 Answers7