views:

201

answers:

7

NOTE: Updated and rewritten

This question has been redone and updated. Please pardon outdated references below. Thanks.

I've seen a lot of javascript code lately that looks wrong to me. What should I suggest as a better code pattern in this situation? I'll reproduce the code that I've seen and a short description for each one:

Code block #1

This code should never evaluate the inner function. Programmers will be confused because the code should run.

$(document).ready( function() { 
  return function() { 
    /* NOPs */
  }
});

Code block #2

The programmer likely intends to implement a self-invoking function. They didn't completely finish the implementation (they're missing a () at the end of the nested paren. Additionally, because they aren't doing anything in the outer function, the nested self-invoking function could be just inlined into the outer function definition.

Actually, I don't know that they intend a self invoking function, because the code is still wrong. But it appears they want a self invoking function.

$(document).ready( (function() { 
  return function() { 
    /* NOPs */
  }
}));

Code block #3

Again it appears the programmer is trying to use a self-invoking function. However, in this case it is overkill.

$(document).ready( function() { 
  (return function() { 
    /* NOPs */
  })()
}); 

Code block #4

an example code block

$('#mySelector').click( function(event) { 
  alert( $(this).attr('id') );

  return function() { 
    // before you run it, what's the value here?
    alert( $(this).attr('id') );
  }
});

Commentary:

I guess I'm just frustrated because it causes creep bugs that people don't understand, changes scoping that they're not grokking, and generally makes for really weird code. Is this all coming from some set of tutorials somewhere? If we're going to teach people how to write code, can we teach them the right way?

What would you suggest as an accurate tutorial to explain to them why the code they're using is incorrect? What pattern would you suggest they learn instead?


All the samples I've seen that have caused me to ask this question have been on SO as questions. Here's the latest particular snippet I've come across that exhibits this behavior. You'll notice that I'm not posting a link to the question, since the user appears to be quite the novice.

$(document).ready(function() {
 $('body').click((function(){
  return function()
  {
   if (counter == null) {
    var counter = 1;
   }
   if(counter == 3) {
     $(this).css("background-image","url(3.jpg)");
     $(this).css("background-position","10% 35%");
     var counter = null;
   }
   if(counter == 2) {
     $(this).css("background-image","url(2.jpg)");
     $(this).css("background-position","10% 35%");
     var counter = 3;
   }
   if(counter == 1) {
     $(this).css("background-image","url(1.jpg)");
     $(this).css("background-position","40% 35%");
     var counter = 2;
   }


  }
 })());
});

Here's how I proposed that they rewrite their code:

var counter = 1;
$(document).ready(function() {
    $('body').click(function() {
        if (counter == null) {
            counter = 1;
        }
        if (counter == 3) {
            $(this).css("background-image", "url(3.jpg)");
            $(this).css("background-position", "10% 35%");
            counter = 1;
        }
        if (counter == 2) {
            $(this).css("background-image", "url(2.jpg)");
            $(this).css("background-position", "10% 35%");
            counter = 3;
        }
        if (counter == 1) {
            $(this).css("background-image", "url(1.jpg)");
            $(this).css("background-position", "40% 35%");
            counter = 2;
        }
    });
});

Notice that I'm not actually saying my code is better in any way. I'm only removing the anonymous intermediary function. I actually know why this code doesn't originally do what they want, and I'm not in the business of rewriting everyone's code that comes along, but I did want the chap to at least have usable code.

I thought that a for real code sample would be appreciated. If you really want the link for this particular question, gmail me at this nick. He got several really good answers, of which mine was at best mid-grade.

+1  A: 

I think a lot of people will agree with your sentiment that sometimes its not good to try and be too cute. If you don't need a language construct, then dont use it, especially if it is confusing.

That said, sometimes you do need these things.

(function(){
    var x = 'test';
    alert(x);
})()

alert(typeof(x)); //undefined

Here, the x variable is locked into the functions scope.

(I found the above example at http://helephant.com/2008/08/javascript-anonymous-functions/)

So, you can do weird things with scope with javascript; this is one way. I agree with your sentiment that a lot js code is more complicated than it needs to be.

hvgotcodes
@drachenstern - Hard to tell if you're being sarcastic.
Coronatus
@drachenstern modified my answer
hvgotcodes
@hvgotcodes ~ Previous comments removed, thanks. Good info too ;)
drachenstern
+1  A: 

Anonymous functions are useful for:

  • Passing logic to another function
  • Declaring single use functions
  • Declaring a function without adding a variable to the scope
  • Providing scope for variables
  • Dynamic programming

You can read more on these topics at the following link: javascript anonymous functions

Michael Goldshteyn
Sorry bro, I already grok anonymous functions. I use them all day. Not my question.
drachenstern
OK, I'll bite. They are learning them from here: http://www.learn-javascript-tutorial.com/AdvancedTechniques.cfm#anonymous-functions
Michael Goldshteyn
I'm sorry, you're still not understanding my question. How is that?
drachenstern
@Michael Goldshteyn ~ Question completely rephrased. Care to delete or modify? I'll remove my comments if so.
drachenstern
A: 

They all appear to be forms of javascript closure's and if you have asynchronous calls happening, they can often be the only way to have the correct value of a variable in the proper scope, when the function finally gets called.

For examples/explanations, see:

Chad
Sorry, nope. A variable using `this` on the first code block in the nested "do stuff here" wouldn't be the same `this` that an inexperienced coder working with closures would expect. Read my code again.
drachenstern
I never said you didn't know what an anonymous function was. I was referring specifically to their ability to hide their scope and maintain state that can only be modified by the closure and not accidentally with other code.
Chad
k, withdrawn. THanks.
drachenstern
@Chad ~ Question completely rephrased. Care to delete or modify? I'll remove my comments if so.
drachenstern
@drachenstern, nope, my comments still stand. I think you're refactoring of the code is wrong. Using a global variable instead of a closure is dangerous.
Chad
I didn't say my refactoring of the code was right. I said it was better than what he had. I didn't rewrite it to be great.
drachenstern
I don't believe yours is better, *global variables are dangerous*. In a closure, the variable is enclosed, contained, safe from other methods changing it. As a global, anything, even another script can modify that variable.
Chad
So I suppose next you're going to refer me to http://www.cs.utexas.edu/users/EWD/ewd02xx/EWD215.PDF ? I mean, look Chad, I wasn't trying to give the poster the best code in the world, I wanted to help him get working code. I know mine has flaws. However, in this particular case, I wasn't particularly worried about the flaws. Considering he was constantly rescoping his variables, do you think he understands closures in the first place? Had he said "hey, my closure isn't working like I expected it to" then I would've gone to the trouble of helping him have a working closure.
drachenstern
+6  A: 

"A little bit of knowledge is a dangerous thing."

Your first example is strange. I'm not even sure if that would work the way the author likely intended it. The second simply wraps the first in unnecessary parens. The third makes use of a self-invoking function, though since the anonymous function creates its own scope (and possibly a closure) anyways, I'm not sure what good it does (unless the author specified additional variables within the closure - read on).

A self-invoking function takes the pattern (function f () { /* do stuff */ }()) and is evaluated immediately, rather than when it is invoked. So something like this:

var checkReady = (function () {
    var ready = false;
    return {
        loaded: function () { ready = true; },
        test: function () { return ready; }
    };
}())
$(document).ready(checkReady.loaded);

creates a closure binding the object returned as checkready to the variable ready (which is hidden from everything outside the closure). This would then allow you to check whether the document has loaded (according to jQuery) by calling checkReady.test(). This is a very powerful pattern and has a lot of legitimate uses (namespacing, memoization, metaprogramming), but isn't really necessary in your example.

EDIT: Argh, I misunderstood your question. Didn't realize you were calling out poor practices rather than asking for clarification on patterns. More to the point on the final form you asked about:

(function () { /* woohoo */ }())
(function () { /* woohoo */ })()
function () { /* woohoo */ }()

evaluate to roughly the same thing - but the first form is the least likely to have any unintended consequences.

C-Mo
Thank you! You're on the same page as me. I didn't know what that block did, and somebody else ID'd it already, so that was helpful. But I agree, I don't know that the people I've seen using it know what it does or why it does it. I don't think that they are being taught correctly.
drachenstern
Also, the first block is literally code taken from a question on this site by a new member (hence my distaste in calling out the one that caught my fancy today and prompted me to ask the question) but I've seen it enough times now that what I thought was a fluke is apparently being taught.
drachenstern
Not only am I calling out poor practices, I would like to shame whoever is teaching new coders these poor practices. I mean, look, I use anonymous functions all day, where they belong. I don't, however, encourage people to do like the first block shows, because they won't understand it.
drachenstern
JavaScript, for better or for worse, has a lot of cargo-cult programmers. That is, they see things being done, and they emulate those things, but don't really understand the underlying idioms. Even otherwise talented developers fall into this trap. This quote from Douglas Crockford seems apropos: "JavaScript is the only programming language I know of that people don't feel the need to learn before they use it."
C-Mo
I think it's one of the patterns discussed in this book http://www.amazon.com/JavaScript-Good-Parts-Douglas-Crockford/dp/0596517742 -- though perhaps not discussed thoroughly enough.
Joel Coehoorn
+1  A: 

I'm not sure if you are recreating your examples exactly or not, but for this

// I picked document ready from jQuery cos you ALL know it, I'm sure ;) 
$('#mySelector').click( function(event) { // this line is all boilerplate, right? 
  alert( $(this).attr('id') ); 

  return function() { // <-- WHAT THE ???? IS THIS ????  ? 
    // before you run it, what's the value here? 
    alert( $(this).attr('id') ); 
  } 
}); 

as well as your first example, the returned anon function never runs. if you were returning false here instead of the function, the default action would be prevented. as is I guess it gets evaluated as event handler returned truish by the browser.

if it were changed to this

// I picked document ready from jQuery cos you ALL know it, I'm sure ;) 
$('#mySelector').click( function(event) { // this line is all boilerplate, right? 
  alert( $(this).attr('id') ); 

  return function() { // <-- WHAT THE ???? IS THIS ????  ? 
    // before you run it, what's the value here? 
    alert( $(this).attr('id') ); 
  } 
}() ); 

the outer function would execute at the time where we're attaching the event handler. the first alert would happen at that time as well as any other preparation taking place in the same scope. the result is to produce the actual method fired during 'onclick', which is the inner function which also has access to the data during the prep phase.

I can see this being appropriate in certain cases although it seems rather convoluted. As for the former example, I'll just assume your programmer has no idea what they are doing.

lincolnk
I think the assumption is "novice programmer" and "cargo cult". You know what tho? I haven't tested some of the code samples I've seen, so I made a claim earlier that they run, and now I don't know that that's true. I think you're right, and I should test it to see if it actually does execute the code. I think it depends on how it gets wrapped (the self-invoking ones should run).
drachenstern
I agree. new programmer adopts the jquery form and runs with it without considering what's actually happening.
lincolnk
A: 

For code block one, that looks a little like a pattern I've seen used for objects with private members. Consider:

function MyObject()  {
   var privateMember = 1;

   return {
        "Add":function(x) {return x + privateMember;},
        "SetPrivateMember":function(x) {privateMember = x;},
        "GetPrivateMember":privateMember
   };
}
Joel Coehoorn
And I'll accept that as valid, if we're talking about creating objects with private members. However the codeblock is about responding to events, in this case, so that's not a valid reason for using this construct. Does that make sense?
drachenstern
+1  A: 

Code block #3 is the only one that is even remotely sane - you don't return a function from an event handler, ever. Self-executing anonymous functions, however, have many uses, like namspacing or private variables. Most of those uses are unnecessary inside an event handler (which is an anonymous function itself), though. Still, they can be used to avoid unwanted side effects of closures:

$(document).ready(function() {
    for (var i = 9; i <= 10; i++) {
        $('#button'+i).click(function() {
            alert('Button '+i+' clicked');
        });
    }
});

this will alert "Button 10 clicked" for all the buttons, but this one will work correctly:

$(document).ready(function() {
    for (var i = 9; i <= 10; i++) {
        (function() {
            var j = i;
            $('#button'+j).click(function() {
                alert('Button '+j+' clicked');
            });
        })();
    }
});
Tgr