Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Recent JavaScript student here. I ran afoul of some weirdness with event listeners that led to my code being unresponsive. After some reading and tweaking, I arrived at a version that worked in JSFiddle, but not in the browser. I finally got it working in the browser, but now my code looks like this:

window.onload = function () {
    function bgnAddChllng() {
        alert("This is a test alert.");
    }
var addChllng = document.getElementById("addChllng");
addChllng.addEventListener('click', bgnAddChllng, false);
}

But the use of window.onload and nesting the actual useful code in an anonymous function seems bloated/hacky to me. What, if anything, can I do to clean it up?

Available on a JSFiddle here.

share|improve this question

3 Answers 3

up vote 4 down vote accepted
window.onload = function () {

Mixing DOM-0 event handlers and addEventListener is inconsistent, pick one and stick with it.

window.addEventListener("load", function () {

function bgnAddChllng() {

Don't disemvowel your variables. This isn't Wheel of Fortune, vowels don't cost anything.

function beginAdditionChallenge() {

var addChllng = document.getElementById("addChllng");

Use proper indentation. JSFiddle has a handy TidyUp button to do it for you. Consider a more descriptive ID for the button.

var button = document.getElementById("challengeButton");

addChllng.addEventListener('click', bgnAddChllng, false);

Switching between single and double quotes is inconsistent, pick one and stick with it. useCapture defaults to false, there's no need to explicitly pass it.

button.addEventListener("click", beginAdditionChallenge);

nesting the actual useful code in an anonymous function seems bloated/hacky to me.

It's a pretty common practice, but if you don't like it, give the function a name and don't nest it, just as you did with the button click event handler.

function beginAdditionChallenge() {
    alert("This is a test alert.");
}

function initChallenge() {
    var button = document.getElementById("challengeButton");
    button.addEventListener("click", beginAdditionChallenge);   
}

window.addEventListener("load", initChallenge);
share|improve this answer
    
Wow, that's very thorough, specific, and helpful. Thank you! My only question in response to all that is about the last bit--between the nesting option and the non-nesting option you provided, which would you recommend? The nested syntax seems odd to me, but I also have less than 10 hours of Javascript experience, so my perception is quite likely groundless. –  user242007 Jun 14 at 7:49
    
@user242007 I actually prefer to avoid anonymous, nested functions when possible; it keeps the code neater (less indentation) and gives more readable stack traces when errors occur. –  Dagg Jun 14 at 7:56
    
@user242007 do keep in mind that doing it this way pollutes the global scope, though. I'm assuming that at some point all of this will end up nested within another function (scope) to keep things out of the global scope. –  Dagg Jun 14 at 7:59
    
Good point, I guess it shows my inexperience that the scoping issue hadn't even crossed my mind. Luckily, you've addressed it before I've done any more work on the project. You're a real life-saver. –  user242007 Jun 14 at 8:10
    
@dagg Nice answer - much more thorough than my own. And also good point (in the comments) about reducing indentation by favoring named functions. The other benefit of named functions is that they encourage writing reusable functions. As for keeping the global scope clean, wrapping everything ub an IIFE fixes that handily. 1 extra level of indentation, sure, but that's a fair trade, I think. –  Flambino Jun 14 at 12:36

There's nothing really wrong with your code (apart from some missing indentation). The structure could be different, but functions are first-class objects; you shouldn't worry about declaring functions any more than you might about declaring any other variable.

Besides, your function isn't really anonymous: You're assigning it to window.load. There's little difference between saying var x = function () {...} and function x() {...}

However, you should stick to one event API; right now you're using both window.load and addEventListener. The latter is the better choice.

window.addEventListener("load", function (event) {
  ...
}, false);
share|improve this answer

If you place your code in a script tag at the end of the body, right before the </body> tag, then the code can just be this:

document.getElementById('addChllng').addEventListener('click', function() {
    alert("This is a test alert.");
});

If you place your code before the addChllng element, then you will have to use some sort of system for detecting when the document is ready before you can get the addChllng element. window.onload is one such way (though it waits for all images to load which isn't necessary). Using window.onload, you could have just this;

window.addEventListener('load', function() {
    document.getElementById("addChllng").addEventListener('click', function() {
        alert("This is a test alert.");
    });
});

If you a bit more extensible system that won't wait until all images are loaded and can be used multiple times for different pieces of code in your project and you aren't using a framework that already has this built into it, then you can use the docReady() function in this answer.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.