Randomizing array elements and picking one in time interval with javascript

Question

I have write piece of code that would randomly pick an element from array, and continiously looping the script in some interval. When I ran it, it works, but browser freeze, so its bad code. Its based on Fisher-Yates algorithm. Here is the code:

function random_notes(x){
    
     var frame = document.getElementById("frame");
     var my_array = ['E', 'G' , 'A', 'C', 'D', 'G#', 'F', 'C#', 'D#', 'A#', 'F#','B'];
     var ri = Math.floor(Math.random() * my_array.length);
     var result = my_array[ri];
    
     var div = document.getElementById('frame');
     div.innerHTML= result;
    
     setTimeout("random_notes()",x*1000); // x represent a seconds
     return random_notes();
    
    }

<input type= "submit" value= "submit" onSubmit="random_notes(2)">

and HTML

Div with ID frame is defined in HTML also. Can you help me to optimize it? Thanks!

your function has an infinite loop - at the end, it tries returning its own return value, recursively forever. Regardless of the timeout. — doldt, Jul 01 '15 at 08:52
Passing a string to `setTimeout` is not a very good idea, see http://stackoverflow.com/questions/6081560/is-there-ever-a-good-reason-to-pass-a-string-to-settimeout — Sam, Jul 01 '15 at 08:54
@doldt good point, and so `setTimeout` will be called with `undefined * 1000` = `NaN` as the timeout meaning `random_notes()` instantly gets called again. That makes for some very exponential recursion. No wonder the browser froze. — Sam, Jul 01 '15 at 08:58
Yea, I know it has infinite looping, and reasons why browser freeze...can someone try to help me to fix it? — fugitive, Jul 01 '15 at 09:10
@MilosM Don't return anything from the function (since it modifies the dom already), and don't use a string in setTimeout - create an anonymous function there which calls random_notes(x) again. — doldt, Jul 01 '15 at 09:11
@doldt after 100x times will have 100x functions on the call stack though — a_kats, Jul 01 '15 at 09:14
@a_kats good point, I was just concentrating on fixing the code without changing the basic structure. The setInterval answer you posted is a better solution. — doldt, Jul 01 '15 at 09:28
I don't see any Fisher-Yates in there. Fisher-Yates is a shuffling algorithm. The code you posted simply selects a random element from the array each time, so it may pick the same element more than once (unlike a shuffling algorithm). — Michael Geary, Jul 01 '15 at 09:48

a_kats · Answer 1 · 2015-07-01T09:19:01.640

0

function random_notes(x, refreshId){

var frame = document.getElementById("frame");
var my_array = ['E', 'G' , 'A', 'C', 'D', 'G#', 'F', 'C#', 'D#', 'A#', 'F#','B'];
var div = document.getElementById('frame');

refreshId = setInterval(function() {

        var ri = Math.floor(Math.random() * my_array.length);
        var result = my_array[ri];

        div.innerHTML= result;

    }, x*1000);

}

Call it with x being the delay, and refreshId a var, so you can stop the loop from the outside. Eg

var tmp;
random_notes(10, tmp);
if (something) clearInterval(tmp);

Edit: moved DOM getElementById outside setInterval to speed things up

edited Jul 01 '15 at 09:19

answered Jul 01 '15 at 09:11

a_kats

191
7

Welcome to Stack Overflow! A couple of tips: 1) It isn't usually necessary to put an "Edit" note when you update an answer - it's common and recommended to freely make updates. 2) The `clearInterval(tmp)` isn't going to work - `tmp` is `undefined` at that point - assigning `refreshId` in the function won't change the value of `tmp`. – Michael Geary Jul 01 '15 at 09:28
I confess I did not test the above code, but I will soon. Arguments are passed by ref, so I guess it will be working. Brb – a_kats Jul 01 '15 at 09:32
JavaScript arguments are indeed passed by reference, but not in the same sense of the word as, for example, C++. When you write `refreshId = ...` inside the function, it doesn't reach back outside the function and change the value of the variable that was passed into the function. It replaces the local `refreshId` variable with a new value - and only that local variable. – Michael Geary Jul 01 '15 at 09:44
What you could do instead is *return* the interval ID from the function. – Michael Geary Jul 01 '15 at 09:45
I see. Thanks for the comment. Btw, the following scenario would work(i think yes) : tmp = {}, outside the function. refreshId.id = setInterval(...). The outer tmp sees(?) the changes made to the object. – a_kats Jul 01 '15 at 09:55
Yes, that would indeed work, and that's where you can take advantage of JavaScript's passing arguments by reference. It is a very useful technique. In this particular code it might be simpler to just return the refreshId, but passing an object into a function that changes some property of the object is very good and useful in many cases. (I would change the variable names here though, not `tmp` outside the function and not `refreshId` inside it.) – Michael Geary Jul 01 '15 at 09:59

score 0 · Answer 2 · edited May 23 '17 at 11:51

0

addEventListener("DOMContentLoaded", function(event) {
  var span_randomElement = document.getElementById("span_randomElement");
  var button_getRandomElement = document.getElementById("button_getRandomElement");
  
  var elements = 'E G A C D G# F C# D# A# E# B'.split(' ');
  
  var multiplier = 1;  
  var seconds = 1000 * multiplier;
  
  var randomNumber = 0;
  var lastRandomNumber = 0;
  
  function start_gettingRandomNumber() {
    setInterval(function(){    
      do {
        randomNumber = Math.floor((Math.random() * (elements.length - 1)) + 0);
      }while(randomNumber == lastRandomNumber);
    
      lastRandomNumber = randomNumber;
      
      span_randomElement.innerHTML = elements[randomNumber];
    
    }, seconds);
  }  
  
  button_getRandomElement.addEventListener("click", function() {
    start_gettingRandomNumber();
    alert("Getting random letter every " + multiplier + " seconds...");
  });
  
  
});

<div>
  <button id="button_getRandomElement">Get a random element</button>
  <span id="span_randomElement"></span>
</div>

I made a script to get random letter on click on a button. I added an addeventListener("DOMContentLoaded") to be sure I am getting elements after they all have been loaded.

EDIT

Added proper "notes" array, I didn't figured out we were talking about music, and reduced the time between each intervals.

EDIT 2

Edited through the advices of Michael Geary

edited May 23 '17 at 11:51

Community

1
1

answered Jul 01 '15 at 09:44

Anwar

4,162
4
41
62

Just a tip, if you'd like a cleaner way to set up that `elements` array, you could do: `var elements = 'E G A C D G# F C# D# A# E# B'.split(' ');` - this is very handy for arrays of this kind. – Michael Geary Jul 01 '15 at 09:51
Another tip... I definitely don't recommend doing this: `var elementsLength = elements.length;` - it just adds more potential for error. Simply use `elements.length` directly where needed. – Michael Geary Jul 01 '15 at 09:53
This is doing a job very well. I will now try to write function to pass in arguments to change multiplier value. Thank you man, you solved my problem! – fugitive Jul 01 '15 at 10:10
You are welcome, don't forget to write the answer that best fits your needs to warn others stackers your problem is solved. – Anwar Jul 01 '15 at 10:17
Hi guys, well I have managed to upload my site recently, and on this link is a tool which couldn't exist with your help! :) Thanks again! http://sixmagicnotes.16mb.com/index.php/random-notes/ – fugitive Jul 03 '15 at 13:53

Randomizing array elements and picking one in time interval with javascript

2 Answers2