Wrote two JavaScript functions but one of them seems to be crashing my website

Question

So I wrote a simple program that should change the background of a given website after 3 seconds.

Now this is my JavaScript code:

//this function changes the backgrounds after 3 seconds and increments n
function changebackgroundTimed(startvariable)
{
    var n = startvariable;
    var loop = 1; 

    while (loop == 1)
    {
        setTimeout(function(){changebackground(n)}, 3000)
        n++;
        if (n == 5)
        {
            n=1;
        }
    }



}

//this function changes the background depending on the given number
function changebackground(number)
{
    if (number == 1)
    {
        $("body").css("background","no-repeat center/120%url('../images/1.jpg')");
    }

    else if (number == 2)
    {
        $("body").css("background","no-repeat center/120%url('../images/2.jpg')");
    }

    else if (number == 3)
    {
        $("body").css("background","no-repeat center/120%url('../images/3.jpg')");
    }
    else if (number == 4)
    {
        $("body").css("background","no-repeat center/120%url('../images/4.jpg')");
    }

    else {
        $("body").css("background","no-repeat center/120%url('../images/1.jpg')");
        }



}

in the html I just call it with: changebackgroundTimed(2);

Problem is: When I start the page it just loads for a long while and then eventually crashes while showing nothing. It has to do something with these two functions. Does anybody of you notices a mistake I may be missing?

Are you changing "loop" anywhere else in your code? You have `while loop === 1...` Loop will always = 1 because you set it prior. You're just stuck in that loop. I would suggest changing that `while` to an `if-else`. — King11, Jan 10 '20 at 15:30
yeah i want the loop to go infinitely so that the background picture just keeps on changing every 3 seconds — , Jan 10 '20 at 15:31
why dont you use setInterval and do that inside of interval? — Armin, Jan 10 '20 at 15:32
`loop` never changes. So `while (loop == 1)` is always true. So he function never stops. — KooiInc, Jan 10 '20 at 15:32
https://stackoverflow.com/questions/3138756/calling-a-function-every-60-seconds — Nico, Jan 10 '20 at 15:33

Abhay H Soningra · Accepted Answer · 2020-01-10T15:52:27.413

5

Looks like you are not updating your "loop" variable, which is causing it go in an infinite loop.

Instead of using the while loop, use setInterval() method. That should do the work for you.

Keep the variable n outside the function, and refer it using outsiders this keyword.

       function abc(){   

       var self = this;
       self.n = 1;

       setInterval(function () {   
            if(self.n ===1){
            self.n = self.n + 1;
            // your code
            }else if(){
            // and so on
            }     
           changebackground(self.n);
       },3000);
      }

edited Jan 10 '20 at 15:52

answered Jan 10 '20 at 15:32

Abhay H Soningra

113
9

oh boy thanks mate i realise now that i wasnt aware what a setInterval function ist, its working now thank you very much :) – Jan 10 '20 at 15:41
1

Upvoted, but `var self = this` looks so pre-2015 :) Now we have ES6 with fat arrow functions and all... Also replace `self.n = self.n + 1` with `self.n++`, things like that. More elegant – Jeremy Thille Jan 10 '20 at 15:52
Fair thing Jeremy, I will keep up with that henceforth, Thanks for pointing it out though :) – Abhay H Soningra Jan 10 '20 at 15:53

Mister Jojo · Answer 2 · 2020-01-10T17:37:35.710

my 2 cents...
CSS:

:root {
  --bg-images :"../images/1.jpg|../images/2.jpg|../images/3.jpg|../images/4.jpg|../images/5.jpg";
  --img-bg    :url(../images/1.jpg);
}
body {
  background-image: var(--img-bg); 
  background-position: center/120%;
  background-repeat: no-repeat;
}

Javascript:

const Root     = document.documentElement
    , gRoot    = getComputedStyle(Root)
    , imgList  = gRoot.getPropertyValue('--bg-images').match(/"(.*?)"/)[1].split('|')
    , NbImgs   = imgList.length
    , regExp = /\(([^)]+)\)/  // to get img url between parentheses
    ;
let currImg = imgList.findIndex(img=>img=== (regExp.exec(gRoot.getPropertyValue('--img-bg'))[1]))
    ;   
setInterval(() => {
  currImg = ++currImg % NbImgs;
  Root.style.setProperty('--img-bg', `url(${imgList[currImg]})`)
}, 3000)

score 0 · Answer 3 · answered Jan 10 '20 at 15:33

0

It appears that you never exit your while loop which makes the page crash as it runs forever. At some part in your code you have to change the value of the loop variable.

answered Jan 10 '20 at 15:33

Aron B.

266
1
6

James Coyle · Answer 4 · 2020-01-10T15:43:29.470

0

You are creating an infinite loop and never breaking out of it so it will run as fast as possible and lock up the UI.

Why not use setInterval like so:

const backgroundChanger = setInterval(changeBackground, 3000)


let background = 1

function changeBackground() {
  if (background >= 5) background = 1

  // set background however you like
  document.body.style.background = 'url(../images/' + background++ + '.jpg) center/120% no-repeat'
}

edited Jan 10 '20 at 15:43

answered Jan 10 '20 at 15:40

James Coyle

9,922
1
40
48

You could also use something like this to not have to worry about when the value is greater than 4: ``$("body").css("background", `no-repeat center/120%url('../images/${number++ % 4 + 1}.jpg')`);`` – GammaGames Jan 10 '20 at 15:43

Armin · Answer 5 · 2020-01-10T15:50:40.150

0

var i = 1;
setInterval(() => {
    if(i > 5){
        i = 0;
    }else{
        changeBackground(i)
    }
    i++;
}, 250);
function changeBackground(i){
    switch(i){
        case 1 :
            $("body").css("color", "red")
        break;
        case 2 :
            $("body").css("color", "blue")
        break;
        case 3 :
            $("body").css("color", "green")
        break;
        case 4 :
            $("body").css("color", "black")
        break;
        case 5 :
            $("body").css("color", "orange")
        break;
        default:
            $("body").css("color", "white")
    }
}

edited Jan 10 '20 at 15:50

answered Jan 10 '20 at 15:41

Armin

373
2
13

You always want to change the background even if you are resetting to the start. With your code the last background will be shown twice as long as the others. – James Coyle Jan 10 '20 at 15:46

score 0 · Answer 6 · answered Jan 10 '20 at 15:46

setTimeout is a non-blocking call. This means that your code wont wait for 3000ms and keep on running in an infinte loop while calling changebackground(n);.

To read more about setTimeout go here setTimeout from MDN

Use the following code:

function changebackgroundTimed(startvariable)
{
 var n = startvariable;
 setInterval(() => {
   changebackground(n);
   n = (n % 5) + 1;
  }, 3000);
}


//this function changes the background depending on the given number
function changebackground(number)
{
 console.log(number)
}

changebackgroundTimed(2)

score 0 · Answer 7 · answered Jan 10 '20 at 16:56

A variation on a theme perhaps but without the hardcoded limitation of the original functions

<!DOCTYPE html>
<html lang='en'>
    <head>
        <meta charset='utf-8' />
        <title>Background image cycling</title>
        <style>
            body{
                width:100%;
                height:100vh;
                padding:0;
                margin:0;
            }
        </style>
        <script>

            /* configuration */
            const PAUSE=3;
            const SLIDES=['//www.stevensegallery.com/800/600','//www.placecage.com/800/600','//placebear.com/640/360','//picsum.photos/800/600'];

            (function( delay ){
                document.addEventListener('DOMContentLoaded',()=>{
                    var images=[/* default images */
                        '//placekitten.com/900/800',
                        '//placekitten.com/1000/800',
                        '//placekitten.com/1024/768',
                        '//placekitten.com/1200/800',
                        '//placekitten.com/1366/768'
                    ];
                    if( arguments[1] )images=images.concat( arguments[1] )
                    var i=0;
                    (function(){
                        setTimeout( arguments.callee, 1000 * delay );
                        i++;
                        if( i > images.length - 1 )i=0;
                        document.body.style.background='center / contain no-repeat url('+images[ i ]+')';
                    })();
                });
            })( PAUSE, SLIDES );
        </script>
    </head>
    <body>
        <!-- content -->
    </body>
</html>

Wrote two JavaScript functions but one of them seems to be crashing my website

7 Answers7