7
\$\begingroup\$

At the moment, I have this code:

(function() {
  var lastSelectedIndex = -1;
  function onSelTypeSelected() {
    // #selType is a <select> tag
    // var selType = document.getElementById("selType");
    var selType = this;
    // finished(onSuccess, onError)
   loader.finished(function(data) {
      lastSelectedIndex = selType.selectedIndex;
    }, function(err) {
      selType.selectedIndex = lastSelectedIndex;
    });
  }
})();

So lastSelectedIndex is not in the global context, however I want to put it into eventListener()'s context. inside eventListener(), so you can't access it in the outer anonymous function. That's cleaner in my opinion and also faster (okay, a few nanoseconds) because of the scope chains hierarchy (Google Tech Talk: Speed Up Your JavaScript is a good introduction).

I thought of adding a property to the function (as I learnt here) but I don't have access to eventListener() in the anonymous functions.

The next idea I had was to use HTML 5 data-* attributes:

function onSelTypeSelected() {
  // #selType is a <select> tag
  // var selType = document.getElementById("selType");
  var selType = this;
  // finished accepts two arguments: onSuccess and onError
  loader.finished(function(data) {
    selType.setAttribute("data-lastIndex", selType.selectedIndex);
  }, function(err) {
    selType.selectedIndex = selType.getAttribute("data-lastIndex");
  });
}

(The default value for data-lastIndex will be set in the HTML Markup)

\$\endgroup\$
1
  • \$\begingroup\$ The code already works. lastSelectedIndex is already in eventListener's context, because they are declared at the same scope. \$\endgroup\$ – delete me Jun 3 '12 at 22:49
7
\$\begingroup\$

lastSelectedIndex is already visible from eventListener. It is in fact visible from everywhere inside the immediately invoked anonymous function.

Try putting console.log("lastSelectedIndex is", lastSelectedIndex); in there and you'll see.

This is due to the concept of closures which in technical-ish terms means that when a variable is not found inside a function's context, it looks in the parent context (the context the function was in when created) and so on all the way up to the window object. In plain-english terms this means that you should forget all your Java/C#/Php training and the variables visible inside a function are exactly those that you intuitively think should be.

That being said, there is going to be only one instance of lastSelectIndex for this block of code, so if the eventlistener is ever attached to more than a single element all instances of the listener will be referencing the same lastSelectIndex.

There are plenty of ways to skin this cat and you really did not give enough details on what your trying to achieve to give advice on best practices but let me try to give some advice anyways

  • DO the simplest thing that can possibly work first
  • DON'T name your functions after how they're used (eventlistner)
  • DO name your functions after what they do
  • DON'T use data- attributes directly. Use a library like jquery, at the very least use element.dataset to retrieve them, though keep in mind the data api is not available in all browsers (which libraries handle for you)
  • CONSIDER having everything set up in response to a user action that triggers it so in pseudocode, the following would be all inline (and with js can be written gracefully and very compact).

    when the user changes the option memoize the previous value query the server on success continue on error restore the previous value

  • Finally, if you do this more than a few times, CONSIDER coming up with an abstraction (possibly a custom control) for this pattern so you don't have to do it every time

\$\endgroup\$
4
  • \$\begingroup\$ Thanks for your answer. I have changed the names and other things in my code in order to simplify it here (e.g. the event listener is called onSelTypeSelected). What I wanted to achieve was to put lastSelectedIndex inside the event listener so you can't access it from the outer function. This will be an app for Windows 8 (IE 10 supports dataset) ;) \$\endgroup\$ – ComFreek Jun 4 '12 at 7:46
  • \$\begingroup\$ @ComFreek onSelTypeSelected is still a name indicating how you're using it, not what it does. What it does seems to merit more a name like setupLoader. A good rule of thumb whenever you name a function is to forget how you're using it and look at it in isolation - imagine other uses that it could be put to and see if the name still makes sense.If you only describe what the function ''does'' then it will. And yes, I know the onNounVerbed thing has been pushed by MS demos, I've talked to lots of MS people - they hate that convention too :) \$\endgroup\$ – George Mauer Jun 4 '12 at 17:39
  • \$\begingroup\$ The function also displays the loaded data, so what do you think about loadAndShowType? \$\endgroup\$ – ComFreek Jun 5 '12 at 15:32
  • \$\begingroup\$ @ComFreek Yeah that sounds more like it. \$\endgroup\$ – George Mauer Jun 5 '12 at 16:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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