JavaScript Review Notes

These are items that any code reviewer should keep in mind when reviewing JavaScript.

CRITICAL

• ALL JAVASCRIPT FILES MUST BE LINT-FREE

  • Note though that you can sometimes confuse XmlLint [1]

• NO NEW SCRIPT FILES SHOULD BE PULLED IN BY THE YAHOO COMBO-LOADER (use these instructions to add the scripts properly)

• ANY DISPLAY OF TEXT INPUT MUST BE TESTED FOR PROPER ESCAPING. If this isn't tested, we don't know that we're safe against HTML injections.

Plain Old JavaScript

• JavaScript blocks should be indented by 4 spaces.

• If 3 or more lines of strings are to be joined together, use the [str1, str2, ...].join("") form.

• 1 and 2 character variable names are to be avoided.
• Regular expression literals should be avoided.

• All public or protected functions, methods, and object properties should be described with JSDoc comment strings and tags.

  • Module-level private variables don't need full JSDoc tags.

• C++ style comments ("//") should only be used inside function bodies.

• Per-line var statements are preferred inside function bodies.

• Function, method, and variable names should follow the PEP 0008 style guide (and no, this is not how the rest of the world does it).

• Any string literal used more than once in a file should be assigned to a CONSTANT (a variable, really) so that the minifier can compile it to a short name.

• functions should always return null not undefined.

• To make conditionals explicit but readable, use the Y.Lang.isXXX methods: Y.Lang.isArray, Y.Lang.isObject, Y.Lang.isValue, and others.

XHTML, CSS, and Styling

• CSS files should come before JavaScript files in the document head.

• CSS url() directives should have the file name surrounded by single-quotes: url('spinner.png')

• XHTML attributes should be surrounded by double-quotes: <input type="text"/>

• Setting a node's style explicitly with setStyle() should be avoided.

• To set a node's style, it is preferable to add a CSS class to the node, and let the stylesheet handle the presentation. [2]

• Please use the W3C markup validation service to check what you write, and to check what the DOM looks like after AJAX inserts new elements.

Event Handling

• "e" should be used as the event object variable name in event handlers: function onclick(e) {...}

• event.halt() should be preferred over event.stopPropagation(), event.preventDefault(), or e.returnValue = false

• Document-wide keypress handlers are dangerous, and should be detached as soon as they are no longer needed.

• Key event handling sucks. The best cross-browser reference we've found to date is at http://unixpapa.com/js/key.html and for an example in our code, see lib/lp/registry/templates/product-new.pt.

Javascript Module formatting

• Modules should have a JSDoc string with the @module or @submodule tag.

• Module-body content does not have to be indented.
• Module-wide constants should go at the top of the file, after the module JSDoc string.

• Multi-line var statements may be used in the module-wide constants block.

• Script tags included for local development should be wrapped in <tal:devmode condition="devmode"></tal:devmode> tags so that un-minified scripts are only loaded for dev usage.

Naming Javascript Modules and Namespaces

• The file path, the module name, and the namespace should correspond to each other.
  • For example:

    • File path: canonical/launchpad/javascript/code/subscription.js

    • Module name: Y.add( "lp.code.subscription" , ...

    • Namespace: var module = Y.namespace( "lp.code.subscription" );

• A module's public functions should be attached to the namespace variable and not be attached directly to the Y.namespace_name variable.

  • Do this:

    var module = Y.namespace("lp.code.subscription");
    module.my_public_function = function() {};
    var my_private_function = function() {};

  • Don't do this, since Y.namespace() will create a new Y.lp object and Y.lp.calendar object if they don't already exist.

    Y.lp.calendar = Y.namespace("lp.calendar");

  • Don't do this, since it will be easier to change the module name and namespace, if you only have to do it in one place in the module.

    var module = Y.namespace("lp.code.subscription");
    Y.lp.calendar.my_public_function = function() {}; // Should use `module` variable here.

• It will then be possible to use module functions like this:

  Y.use("lp.code.subscription", function() {
      Y.lp.code.subscription.my_public_function();
  });

Widgets and Plugins

• Protected and private method names should be prefixed with an underscore.

• All protected methods should have the @protected JSDoc tag, even ones like initializer() and renderUI().

• Event handler method names should be prefixed with _on or _after.

• Custom events should be documented on the line immediately preceding the line where they are published.

• All JSDoc strings for attributes should have @attribute, @type, and @default tags.

• Objects such as Node instances should not be instantiated in the widget's ATTRS hash using value:, use valueFn: instead.

• All Widgets should have a functional example page.

• (Not implemented) Widget example pages should follow the Example page Template.

Tests

• The unit test suite should leave a clean DOM tree in between each test case.

Footnotes

[1] It looks like literal regexps confuse it, but OTOH, we recommend against using them.
Wrong, XmlLint rightly complains there is an unescaped less-than (<) in a document. Use != 0 or != 1 if you must embed script, but the right thing to do is to extract the script to a separate file.

== XmlLint notices ==

lib/lp/registry/templates/product-new.pt
    110: parser error : StartTag: invalid element name
    if (steps.search(/hidesearch/) < 0) {
    ^

[2] For example, if you want to toggle an element's visibility using POJS, use our unseen class:

    // Make this element invisible.
    element1.addClass('unseen');
    // Make this element visible again.
    element2.removeClass('unseen');

CategoryJavaScript