Algorithm stops and I don't have no idea why

Question

Hey! I am trying to code Pexeso with HTML, CSS and Javascript. I have random generating function (number can't be used more then one time - I solved this with the array). Then I have another function which calls the mentioned function and choose by the returned number id of img element and sets image source. Everything looks right - IDE and Google Chrome don't return any errors, but it still doesn't do the right thing - it never source all images.

I am putting the code and also screen how it looks after compiling.

HTML

<!DOCTYPE html>
<html lang='cs'>
  <head>
    <title></title>
    <meta charset='utf-8'>
    <link href='style.css' rel='stylesheet' type='text/css'>
  </head>
  <body>
  <div class="container">
    <img class="pexeso" src="" id="1">
    <img class="pexeso" src="" id="2">
    <img class="pexeso" src="" id="3">
    <img class="pexeso" src="" id="4">
    <img class="pexeso" src="" id="5">
    <img class="pexeso" src="" id="6">
    <img class="pexeso" src="" id="7">
    <img class="pexeso" src="" id="8">
    <img class="pexeso" src="" id="9">
    <img class="pexeso" src="" id="10">
    <img class="pexeso" src="" id="11">
    <img class="pexeso" src="" id="12">
    <img class="pexeso" src="" id="13">
    <img class="pexeso" src="" id="14">
    <img class="pexeso" src="" id="15">
    <img class="pexeso" src="" id="16">       
  </div>
  <script src='app.js'></script>
  </body>
</html>

JS

var position;
position = new Array();

function generateNumber(){
//generates random number between 0 and 15
var number = Math.round(Math.random()*15)+1;

//checks if number is already used
for (var i = 0; i <= position.length; i++){
    if (position[i] == number){
        generateNumber();
    }

//returns number
    console.log(number);
    position.push(number);
    return number;
}
}

function mixPexeso(){
for (var i = 0; i <= 7; i++){
//sets images to their location and type their position
    var firstImage = generateNumber();
    var secondImage = generateNumber();
    var image = 'img/image-'+i+'.png';

    document.getElementById(firstImage).src = image;
    document.getElementById(secondImage).src = image;
    }
}

mixPexeso();

Screen: https://prnt.sc/l4wr1q

`if (position[i] == number){ generateNumber(); }` probably needs to be `if (position[i] == number){ number = Math.round(Math.random()*15)+1 }` I would have thought. Otherwise you're throwing away the result of the new attempt at generating a number. But also I don't think you need to re-run the entire generateNumber() function, that seems overkill, you just need a new random number — ADyson, Oct 11 '18 at 15:07
there are better ways of generating numbers randomly in a range and that they are only used once. Generate an array of the available numbers, randomly sort it, pick off the first index. — epascarello, Oct 11 '18 at 15:09

ibrahim mahrir · Accepted Answer · 2018-10-11T15:23:43.053

1

The problem:

When you find out that the number already exist, you should break the loop and return the result of generateNumber:

if (position[i] == number) {
    return generateNumber();
}

Alternative way:

Anyways, the function generateNumber could be implemented in a better way by removing the recursive calls and wrapping the whole thing in an IIFE for encapsulation:

var generateNumber = (function() {
  var numbers = [];

  return function() {
    var number;
    do {
      number = Math.floor(Math.random() * 16);
    } while(numbers.includes(number));
    numbers.push(number);
    return number;
  }
})();

Best way:

An even better way is to prefill the array with numbers (0 through 15), shuffle them and then each time the function gets called, you just pop or shift a number from that array:

var generateNumber = (function() {
  var numbers = [];

  for(var i = 0; i < 16; i++) numbers.push(i);

  for(var i = 0; i < 16; i++) {
    var randIndex = Math.floor(Math.random() * 16);
    var temp = numbers[i];
    numbers[i] = numbers[randIndex];
    numbers[randIndex] = temp;
  }

  return function() {
    return numbers.pop();
  }
})();

edited Oct 11 '18 at 15:23

answered Oct 11 '18 at 15:16

ibrahim mahrir

31,174
5
48
73

2

I used the third option and it works, thank you so much. However, I would like to understand the whole code. I understand everything except the return function and round brackets in the end. Sorry If I sound stupid, but I am new in the javascript and I haven't met with this before - I was always good with simple return, which wasn't in his own function. – ondrejhadrava Oct 11 '18 at 18:45
@ondrejhadrava You're welcome! Those are called IIFE (**I**mmediately **I**nvoked **F**unction **E**xpression). It is not necessary for the code to work. The concept is that the function that contains all the code, is immediately called (hence the round brackets at the end), and its return value is assigned to `generateNumber`, hence the return of a function. The code is wrapped in that IIFE as a means to encapsulate the variables from the outside scope (the variable `numbers` to be exact). – ibrahim mahrir Oct 11 '18 at 21:15
@ondrejhadrava In your code, you defined `position` as a global variable, but it is only used by `generateNumber`. It's better to hide it from the global scope and make it accessible only to `generateNumber`, don't you think? We can't just declare it inside `generateNumber`, because that won't work for obvious reasons. So we wrap it along with `generateNumber` inside another function (the IIFE). We could've given the IIFE a name, and called it like a normal function, but it only needs to be called once, so why bother? Therefore we just call it immediately after it is defined. – ibrahim mahrir Oct 11 '18 at 21:16
@ondrejhadrava The function returned from this IIFE is what will be `generateNumber`, we don't really need to name it so it remains anonymous. It will be assigned to the variable `generateNumber` once the IIFE is called, so we'll use this variable (`generateNumber`) from the outside to call it. This function has access to the variable `numbers` due to closures. – ibrahim mahrir Oct 11 '18 at 21:19
@ondrejhadrava more on IIFEs in this [other SO question](https://stackoverflow.com/questions/8228281/what-is-the-function-construct-in-javascript). Read also about the ***module pattern***, which is what IIFEs are mainly used for, [here](https://stackoverflow.com/questions/17776940/javascript-module-pattern-with-example). Good luck! – ibrahim mahrir Oct 11 '18 at 21:28

Algorithm stops and I don't have no idea why

1 Answers1