Javascript code not running inside onload function

Question

I am wondering why this code does not work. When I put it inside the onload function, it does not work, and I am not sure why. I have a lot of other code in the onload function, but I have deleted it for this example. Anyway the other code works fine, only this part does not.

window.onload = function() {
var i, timer, divide;
i = 0;
divide = 100;

function start() {
    timer = self.setInterval("increment()", (1000 / divide)); 
}

function increment() {
    i++;
    document.getElementById("timer_out").innerHTML = (i / divide);
}

function stop() {
    clearInterval(timer);
    timer = null;
}

function reset() {
    stop();
    i = 0;
    document.getElementById("timer_out").innerHTML = (i / divide);
}

}

<div>
    <span id="timer_out">0</span>
    </br>
    <input type="button" value="Start" onclick="start()"/>
    <input type="button" value="Stop" onclick="stop()"/>
    <input type="button" value="Reset" onclick="reset()"/>
</div>

Have you checked your console? (Hit F12 to open it). Your answer lies in there. Basically, your functions need to be global to access them in your `onclick` handlers. A better approach would be to use `.addEventListener`. — Mike Cluck, Feb 16 '16 at 16:50
Basically what @MikeC trying to say is, your every buttons' `onclick` event is unable to call its function since you wrap their function inside `onload` event. The thing is, your buttons load before its function declared. — choz, Feb 16 '16 at 16:53

millerbr · Accepted Answer · 2016-02-16T17:14:20.590

10

You need to define your functions and variables outside the onload scope, otherwise they aren't accessible.

var i, timer, divide;
function start() {
    if(!timer) {
        timer = setInterval("increment()", (1000 / divide)); 
    }
}

function increment() {
    i++;
    document.getElementById("timer_out").innerHTML = (i / divide);
}

function stop() {
    clearInterval(timer);
    timer = null;
}

function reset() {
    stop();
    i = 0;
    document.getElementById("timer_out").innerHTML = (i / divide);
}
window.onload = function() {
    i = 0;
    divide = 100;
}

As mentioned in comments, you also have a bug that will make it impossible to stop the timer if you click start more than once. This is because you are creating two (or more) timers but only have a reference to the last one. To fix this, a simple if statement checking if a timer exists should suffice. Code above has been updated to show this, and here is an example of it running: http://jsfiddle.net/qPvT2/40/

edited Feb 16 '16 at 17:14

answered Feb 16 '16 at 16:54

millerbr

2,951
1
14
25

Make it a snippet so it'd be pretty to look at :) – choz Feb 16 '16 at 16:56
Or better yet, avoid adding even more globals to the already-overcrowded global namespace by hooking up the elements with modern techniques. – T.J. Crowder Feb 16 '16 at 16:57
Hmm this is weird i have tried this and it has not worked – Blindeagle Feb 16 '16 at 16:57
1

@Blindeagle: You still have them inside your `onload` in that fiddle. (Hint: Consistent indentation makes it easier to see things like that.) – T.J. Crowder Feb 16 '16 at 16:58
2

@Blindeagle The code you linked to doesn't contain the fix. Also, you'll want to remove `self.` since you don't declare `self`. Finally, avoid using strings with `setInterval`. Instead, just write `setInterval(increment, 1000 / divide)` – Mike Cluck Feb 16 '16 at 16:59
1

Good eye on the `self` catch, removed that from my answer – millerbr Feb 16 '16 at 17:00
@silviagreen yes it is working, your fiddle is wrong http://jsfiddle.net/qPvT2/37/ – Adam Buchanan Smith Feb 16 '16 at 17:00
Sorry i should have explained my self better. That code there works perfect. When i add my code all together it does not work one bit, – Blindeagle Feb 16 '16 at 17:02
This solved OP's issue. But I notice that if you click the `start` button multiple times, you won't be able to stop it. – choz Feb 16 '16 at 17:02
The error i get is : 615 clock.js:189 Uncaught TypeError: Cannot set property 'innerHTML' of null, is this because i have them in two different files? – Blindeagle Feb 16 '16 at 17:02
For example i have my html file and then i have my javascript file but again i dont know why this is a problem – Blindeagle Feb 16 '16 at 17:03
@Blindeagle That won't happen from them being in separate files. Make sure that the string you're padding to `document.getElementById` matches the ID of the element in the HTML. This code works perfectly. – Mike Cluck Feb 16 '16 at 17:05
1

@MikeC ur a beautiful man! Thank you so much for all the help really happy x – Blindeagle Feb 16 '16 at 17:09
1

As mentioned by @choz you had a bug regarding the start button being pressed more than once. I've updated my answer with a fix for this too. – millerbr Feb 16 '16 at 17:14

score 3 · Answer 2 · edited May 23 '17 at 11:52

Problems:

Functions referened from onxyz attributes must be globals. Your functions aren't globals, because they're declared inside your onload handler. Some people will tell you to make them globals, but there's a better answer.
onload is almost certainly much, much later than you'd really like this code to run. It waits until all images have fully-downloaded, for instance.
Using strings with setTimeout/setInterval is usually not a good idea.
</br> is invalid HTML. In HTML, a br element is written <br>. You can also write it <br/> or <br /> if you like, but the / is meaningless in HTML. (It has meaning in XHTML.)
There's no need to prefix setTiemout/setInterval with self.. It works, because there's a default self global, but it's unnecessary.
If you click start twice, you'll overwrite the previous timer's handle and not be able to stop it. We need to call stop before starting.

See comments for how I've addressed those below.

Without jQuery (because you tagged it, but don't seem to be using it)

// This script is at the end of the HTML, just beofre the closing
// </body> tag, ands o all the elements defined by the HTML above it
// will be there and ready for us to handle. No need for `onload`.
// But we'll use a scoping function to avoid creating globals.
(function() {
  var i, timer, divide;

  // Hook up our buttons.
  // For a very simple thing like this, we can just assign to 
  // their onclick properties, but for anything more complicated
  // I'd use addEventListener (falling back to attachEvent if
  // the browser doesn't have addEventListener, to support obsolete
  // IE like IE8), but that's overkill for this small example.
  document.querySelector("input[value=Start]").onclick = start;
  document.querySelector("input[value=Stop]").onclick = stop;
  document.querySelector("input[value=Reset]").onclick = reset;
  
  i = 0;
  divide = 100;
  timer = 0; // A value we can safely pass to clearInterval
             // if we never set anything else

  function start() {
    // Make sure to stop any previous timer
    stop();
    
    // Note no quotes or (), we're passing a reference to the
    // increment function into `setInterval`.
    // Also no need for `self.`
    timer = setInterval(increment, (1000 / divide));
  }

  function increment() {
    i++;
    document.getElementById("timer_out").innerHTML = (i / divide);
  }

  function stop() {
    clearInterval(timer);
    timer = null;
  }

  function reset() {
    stop();
    i = 0;
    document.getElementById("timer_out").innerHTML = (i / divide);
  }

})();

<div>
  <span id="timer_out">0</span>
  <br>
  <input type="button" value="Start" />
  <input type="button" value="Stop" />
  <input type="button" value="Reset" />
</div>

For more about addEventListener/attachEvent and a function you can use if you need to support obsolete browsers, see this answer.

With jQuery (because you did tag it)

// This script is at the end of the HTML, just beofre the closing
// </body> tag, ands o all the elements defined by the HTML above it
// will be there and ready for us to handle. No need for `onload`.
// But we'll use a scoping function to avoid creating globals.
(function() {
  var i, timer, divide;

  // Hook up our buttons.
  $("input[value=Start]").on("click", start);
  $("input[value=Stop]").on("click", stop);
  $("input[value=Reset]").on("click", reset);
  
  i = 0;
  divide = 100;
  timer = 0; // A value we can safely pass to clearInterval
             // if we never set anything else

  function start() {
    // Make sure to stop any previous timer
    stop();
    
    // Note no quotes or (), we're passing a reference to the
    // increment function into `setInterval`.
    // Also no need for `self.`
    timer = setInterval(increment, (1000 / divide));
  }

  function increment() {
    i++;
    $("#timer_out").text(i / divide);
  }

  function stop() {
    clearInterval(timer);
    timer = null;
  }

  function reset() {
    stop();
    i = 0;
    $("#timer_out").text(i / divide);
  }

})();

<div>
  <span id="timer_out">0</span>
  <br>
  <input type="button" value="Start" />
  <input type="button" value="Stop" />
  <input type="button" value="Reset" />
</div>
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>

Same thing as miller's answer, click `start` twice and won't be able to stop it. — choz, Feb 16 '16 at 17:07
Ah did not see this problem , if u knoy any way to fix it, please let me know — Blindeagle, Feb 16 '16 at 17:12
@Blindeagle: It's fixed in the answer now (hit refresh just in case). — T.J. Crowder, Feb 16 '16 at 17:13

score 3 · Answer 3 · answered Feb 16 '16 at 17:38

3

Instead of writing your functions in window.onload.. Just write them outside of it... As you are writing them inside the window.onload function, they are not accessible...just write them outside and your code will run properly

answered Feb 16 '16 at 17:38

Milind Shinde

31
2

Javascript code not running inside onload function

3 Answers3

Without jQuery (because you tagged it, but don't seem to be using it)

With jQuery (because you did tag it)