Two recursive functions try to execute at the same time when only one should run

Question

Essentially, I am trying to create a webpage that has 2 buttons; one will cause the screen to flash randomly, and the other will cause it to slowly change colours. I want to be able to switch between these two (if you press the first button it starts flashing and then if you press the second button it slowly changes without any kind of cancel button).

The buttons each call a function which sets the 'running' variable of the other function to false and it's own 'running' variable to true. It then calls a recursive function (recursive in that it just calls itself over and over). These recursive functions only execute their code when their 'running' variable is true.

If you run the snippet you can see that the program is very inconsistent (you may need to play with it for a bit to see the issue since it sometimes seems to work). Sometimes it refuses to change function and other times the two functions seem to both be active and they both try to execute (it almost looks as if they are fighting for control). I don't understand how this is happening since, I believe, only one of the 'running' variables can be true at any time.

var runningDisco = false;
var runningColours = false;


function startColours() {
  if (runningDisco == true); //Is disco running?
  {
    runningDisco = false; //If yes, stop it
  }
  runningColours = true; //Indicate we are running
  window.setTimeout(Colours, 100, 0); //Run
}

function startDisco() {
  if (runningColours == true); {
    runningColours = false;
  }
  runningDisco = true;
  window.setTimeout(Disco, 100);
}

function Disco() {
  if (runningDisco == true); {
    hex = "#";
    for (discoCount = 0; discoCount < 6; discoCount++) {
      hex = hex.concat((Math.floor(Math.random() * 17)).toString(16));
    }
    document.body.style.background = hex;
    window.setTimeout(Disco, 10);
  }

}

function Colours(colourCount) {
  if (runningColours == true); {
    if (colourCount > 359) {
      colourCount -= 359;
    }
    document.body.style.background = "hsl(" + colourCount + ", 50%, 50%)";
    window.setTimeout(Colours, 10, colourCount + 1);
  }

}

input {
  font-family: 'Roboto', sans-serif;
  text-align: center;
  background-color: #dd1021;
  border: none;
  color: white;
  padding: 16px 32px;
  text-decoration: none;
  margin: 4px 2px;
  cursor: pointer;
}

<link href="https://fonts.googleapis.com/css?family=Roboto" rel="stylesheet">

<input id="clickMe" type="button" value="Start Disco" onclick="startDisco();" />
<input id="clickMe" type="button" value="Start Colours" onclick="startColours();" />

I don't understand your title. is it a question or what? please make the title clearer — vsync, May 11 '17 at 08:21
Running the same function with `setTimeout` is not really recursion. — Barmar, May 11 '17 at 08:22
Yeah, that's why I said _recursive in that it just calls itself over and over_ .not really sure what the correct name for something like this is — KangarooChief, May 11 '17 at 08:23

Yosef Weiner · Accepted Answer · 2017-05-11T08:33:52.100

5

The semicolons after your if-statements are causing a problem

For example, replace:

 if (runningColours == true); {
 if (runningDisco == true); {

with

 if (runningColours == true) {
 if (runningDisco == true) {

Here it is fixed in jsfiddle.

edited May 11 '17 at 08:33

answered May 11 '17 at 08:24

Yosef Weiner

5,432
1
24
37

Yes, I appear to have added in an if statement and forgotten to remove them, then copied the if statement elsewhere – KangarooChief May 11 '17 at 08:27
1

Actually, no. With or without the semicolon, the result is always the same - `runningColours` is set to `false` after this. – Bergi May 11 '17 at 08:28
@Bergi in the "start" functions you are correct, but in the `Colours` and `Disco` functions, this is what is causing the problem – Yosef Weiner May 11 '17 at 08:34
I see. But your answer didn't state that. – Bergi May 11 '17 at 08:36
@Bergi the answer stated: "...the semicolons after your if-statements are causing a problem" which was accurate. The problem was that the OP was putting semicolons after if statements. – Yosef Weiner May 11 '17 at 08:38
1

ONE problem was that. A LOGICAL problem is not clearing the intervals when switching – mplungjan May 11 '17 at 09:10
@mplungjan true, but once the if() statements were fixed, the code was defensive enough so that there would be no effect to not clearing the intervals – Yosef Weiner May 11 '17 at 09:17
Because everything is inside an if. Much more elegant to use a timeout handler. – mplungjan May 11 '17 at 09:21
@mplungjan absolutely agreed. But not a bug. – Yosef Weiner May 11 '17 at 10:31

score 2 · Answer 2 · edited May 11 '17 at 09:11

You have semicolons in the wrong place
If you need to run a single animation you need to use a single global variable to hold the timeout handler and clear it when starting a new one.

Like this:

var running;

function startColours() {
  clearTimeout(running);
  running = setTimeout(Colours, 100, 0); //Run
}

function startDisco() {
  clearTimeout(running);
  running = setTimeout(Disco, 100);
}

function Disco() {
    var hex = "#";
    for (discoCount = 0; discoCount < 6; discoCount++) {
      hex = hex.concat((Math.floor(Math.random() * 17)).toString(16));
    }
    document.body.style.background = hex;
    running = setTimeout(Disco, 10);

}

function Colours(colourCount) {
    if (colourCount > 359) {
      colourCount -= 359;
    }
    document.body.style.background = "hsl(" + colourCount + ", 50%, 50%)";
    running = setTimeout(Colours, 10, colourCount + 1);
}

input {
  font-family: 'Roboto', sans-serif;
  text-align: center;
  background-color: #dd1021;
  border: none;
  color: white;
  padding: 16px 32px;
  text-decoration: none;
  margin: 4px 2px;
  cursor: pointer;
}

<link href="https://fonts.googleapis.com/css?family=Roboto" rel="stylesheet">

<input id="clickMe" type="button" value="Start Disco" onclick="startDisco();" />
<input id="clickMe" type="button" value="Start Colours" onclick="startColours();" />

It's a nice suggestion but has nothing to do with the problem the OP was asking about — Yosef Weiner, May 11 '17 at 08:56
@SkinnyJ Not really. Original problem OP is trying to solve was "... I want to be able to switch between these two". His code is XY-problem. And I provide a better solution to the original problem. Otherwise this question should be closed as it was just a typo. — Yury Tarabanko, May 11 '17 at 09:00

Benedict Bunting · Answer 3 · 2017-05-11T08:40:37.007

0

You made a common mistake to those new to JavaScript - including a semi-colon after you if header. This creates an if statement with no body that is followed by a block containing the single statement runningColours = false, which will not execute correctly. The below should fix this:

if (runningColours == true)
{
    runningColours = false;
}

The following JSFiddle link should have a working version of your script: https://jsfiddle.net/4qhmtnhe/ I hope it starts working, and you continue to learn JavaScript.

edited May 11 '17 at 08:40

answered May 11 '17 at 08:32

Benedict Bunting

318
2
8

The if statement does nothing and the code in the bracers is always executed. – Dimitri L. May 11 '17 at 08:47
This is more an issue in the recursive functions, Disco and Colours, as it means that they continually run. – Benedict Bunting May 11 '17 at 18:04

score 0 · Answer 4 · edited May 23 '17 at 12:17

The real problem is the semicolons after the if statements inside the Disco and Colours functions:

if (runningDisco == true); {
  document.body.style.background = …
}

will execute as

if (runningDisco == true)
  ;
{
  document.body.style.background = …
}

which runs the content regardless of the boolean variable. Why does it work at all sometimes? I guess when the two setTimeout callbacks run close enough to each other that only the latter is rendered at all it feels consistent. Of course that is very unstable - they are indeed fighting each other for screen time.

Two recursive functions try to execute at the same time when only one should run

4 Answers4