0

I'm trying to get 8 random numbers between 1-80 and put them into specified divs.

var numbers = []
var luckyNumber;
var x = 1;

while (x < 8) {
  luckyNumber = Math.floor(Math.random() * 80 + 1);
  if (numbers.indexOf(luckyNumber) == -1) {
    for (var i = 1; i < 9; i++) {
      document.getElementById('k' + i).innerHTML = numbers[i]
    }
    numbers.push(luckyNumber);
    x++;
  }
}

document.write(numbers);
<div id="k1">K1</div>
<div id="k2">K2</div>
<div id="k3">K3</div>
<div id="k4">K4</div>
<div id="k5">K5</div>
<div id="k6">K6</div>
<div id="k7">K7</div>
<div id="k8">K8</div>

The last 2 numbers are shown undefined in HTML result. But if you check the numbers with document.write you can see the last two numbers. How can I solve this problem?

Improve this question
1
  • The given code already working in fiddle. I believe some edit happened. Commented Feb 17, 2017 at 12:07

2 Answers 2

Reset to default
1

Your looping logic seems to be incorrect. You should fill numbers array first and then update the html.

var numbers = []
var luckyNumber;
var x = 1;

while (x < 9) {
  luckyNumber = Math.floor(Math.random() * 80 + 1);
  if (numbers.indexOf(luckyNumber) == -1) {
    numbers.push(luckyNumber);
    x++;
  }
}
for (var i = 1; i < 9; i++) {
      document.getElementById('k' + i).innerHTML = numbers[i-1]
    }

document.write(numbers);
<div id="k1">K1</div>
<div id="k2">K2</div>
<div id="k3">K3</div>
<div id="k4">K4</div>
<div id="k5">K5</div>
<div id="k6">K6</div>
<div id="k7">K7</div>
<div id="k8">K8</div>

Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

Code should have some accompanying text explaining what the problem was
Yes, of course, I missed to see the x increment. Thanks @JJJ.
I do prefer the two pass approach; However I would just use a forEach, or use numbers.length in the second loop instead of hardcoding 9 twice
0
  • The while loop goes from 1 to 7, generating 7 numbers instead of 8.
  • Arrays in JavaScript are 0-indexed, so when you go through the array in the for loop that starts from 1 you're skipping the first number.
  • You're re-filling the divs every time you generate a new number with the old numbers, excluding the new number. The last generated number will never be included in this process.

The simplest fix is to fix the while loop, remove the for loop and use x to find the div where to display the latest number.

var numbers = []
var luckyNumber;
var x = 1;

while (x <= 8) {
  luckyNumber = Math.floor(Math.random() * 80 + 1);
  if (numbers.indexOf(luckyNumber) == -1) {
    document.getElementById('k' + x).innerHTML = luckyNumber;
    numbers.push(luckyNumber);
    x++;
  }
}

document.write(numbers);
<div id="k1">K1</div>
<div id="k2">K2</div>
<div id="k3">K3</div>
<div id="k4">K4</div>
<div id="k5">K5</div>
<div id="k6">K6</div>
<div id="k7">K7</div>
<div id="k8">K8</div>

Improve this answer

5 Comments

Last part of your answer is not clear for me. Can you please explain it again ? Why my loop can't write into last two divs instead of first and last one. It's missing the first and last random number but last two divs are undefined.
Because the last time the for loop runs, the numbers array contains only 6 elements at indexes 0-5. Therefore divs 6-8 are filled with "undefined" because there's nothing else in the array.
I understood. I've put the "numbers.push(luckyNumber);" before for loop, changed the "numbers[i]" to numbers[i -1] and it works now. But i think running the for loop 8 times is not efficient way to do this. So which one is the fastest way your or xitter's ?
One loop is faster than two.
After running the code several times. I realized that sometimes one ore two divs show nothing. I think it's because of if condition "numbers.indexOf(luckyNumber) == -1" . It finds the number inside the array and leaving the div empty. How can I solve it ?

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.