The JavaScript Way

15 JavaScript Development No-Nos

A few weeks ago Chad Fowler posted a list of 20 Rails No-Nos which included 20 of the things people in the Rails community considered to be “wrong”. I thought it would be interesting to compile a similar list for JavaScript code and came up with 15 points I myself consider “wrong”. The list is of course in no way canonical, I’m sure not everyone agrees with all the points or would like some others to be in there. I could only come up with 15, but I’m sure others can come up with some additional points in the comments :)

1) Global variables

Like in most programming languages, using global variables in JavaScript is very much a no-no. They cause unpredictable behaviour and hard-to-track-down errors and have the potential to wreak havoc with not just your own code but other people’s code too. There are several ways in which you can intentionally or accidentally create a global variable in JavaScript:

Creating a variable while in the global context

If you’re not inside a function definition you’re in the global context, and creating a variable here will make it global.

var foo = "global"; //Don't do this

function(){
  var bar = "local"; //This is ok
}();

Not using the var keyword

Any time you assign a variable without using the var keyword, that variable becomes global:

function(){
  foo = "global"; //Don't do this
  var bar = "local"; //This is ok
}();

Assigning properties to the window variable

Properties assigned to the global window variable are the same as global variables:

function(){
  window.foo = "global";//Don't do this
}();

function(){
  return foo; //Returns "global"
}();

Creating global functions

Named functions in JavaScript are function objects assigned to variables. This means that whenever you do something like this…

function foo(){
  //...
};

…you’re creating a global variable named foo.

If this article were limited to only one point it would be to always use var. This is the one thing that’s the easiest to remember and has the most significance when writing JavaScript code.

2) Inline JavaScript

Inline JavaScript is any JavaScript code that is mixed with HTML. There are two primary ways in which you can do this:

script tags

<form id="foo">
<!-- ... -->
</form>
<script>
$('foo').observe('submit', function(e){
  if (!confirm("Are you sure?")) { e.stop(); }
});
</script>

onclick and friends

All HTML elements support at least some events using onfoo handler attributes, and additionally you can place javascript in a link element’s href tag:

<form id="foo" onsubmit="if (confirm("Are you sure?")) { return true; } else { return false; }">
<input type="text" value="Name" onclick="this.value='';" />
<a href="javascript:$('foo').submit()">Submit</a>
</form>

All of the above examples will work fine, but are for several reasons outdated and very poor techniques. For one, it’s just really messy; keeping HTML and JavaScript separate just makes everything a lot cleaner and easier to deal with. Secondly, they’re not reusable.. A better way would be to mark elements using IDs and class names, this way you could write the JavaScript once and reuse it just by adding class names to your HTML elements.

3) eval

Also like most other programming languages, using eval in JavaScript is considered.. well, evil. If you find yourself using eval, there’s a big chance your approach is wrong. The one exception to this is parsing JSON data, which is easily done using eval, but it’s much better to use a library for this, such as Douglas Crockford’s json2.js.

4) Old-style event handling

In the olden days, people would use two primary techniques to do event handling:

The good old onfoo HTML attributes:

<a href="#" id="foo" onclick="handleFooClick();">Click me</a>

And the slightly better JavaScript property functions using the same names:

var el = document.getElementById('foo');
el.onclick = function(){
  handleFooClick()
};

Luckily things have improved since then and we can make use of modern browsers’ more advanced event handling. This means we can keep our event handlers separate from the HTML and we can attach several handlers to each element, prevent the default action from being performed and make use of event capture and bubbling. Internet Explorer, of course, does things its own way, but libraries such as Prototype and jQuery take care of that for us:

//Prototype
el.observe('click', function(e){
  e.stop(); //Prevent the default action
  if (confirm("Are you sure?")) {
    doSomething();
  }
});

5) Using constructors for literal values

JavaScript has a few “literal” values that are shorthand for instantiating objects and adding properties to them. The most-often misused (or not-used) of these are the array and object literals. There’s no need to use the Array and Object constructors:

var list = new Array();
list.push(1);
list.push(2);
list.push(3);

var map = new Object();
map["foo"] = 1;
map["bar"] = 2;

The above can be written more succinctly using literals:

var list = [1,2,3];
var map = {foo: 1, bar: 2};

6) alert("In your face")

Most of the time, using the alert method is unnecessary. The most prevalent misuse of this method is for debugging purposes, but using tools such as Firebug and equivalents for other browsers is a much better and more efficient way to debug JavaScript code.

7) element.style

You can change an element’s style directly by changing properties on its style property:

el.style.backgroundColor = "#ff0000";
el.style.color = "#00ff00";

This is usually not a good idea because you want to keep the styling separate from the behaviour and changing an element’s style directly will override anything you’ve defined in your CSS. A much better way to do this is to assign class names to elements and leave the styling up to the stylesheet:

var el = $$('.post').first();
el.addClassName("marked");
.post.marked {
  background-color: #ff0000;
  color: #00ff00;
}

This way your JavaScript only has to decide if an element should be in a “marked” state or not, and the CSS can decide what that means in terms of styling. When you come back to this code a month later because the definition of “marked” has changed, you (or someone else!) will find what they’re looking for in the most logical place, which is in the CSS stylesheet and not hidden deep inside some JavaScript code. If your new colleague gets tasked with changing this, he’s likely to use Firebug to see what exactly happens when a post gets marked. He will then see that the “marked” class name gets added, and Firebug will additionally tell him exactly in which CSS file and on which line this rule is defined.

8) No namespacing

This is similar to the “no globals” rule because the easiest way to avoid polluting the global namespace is to create your own. A site will usually create one global variable which serves as its namespace:

var MySite = {};
MySite.Foo = Class.create(/* ... */);

This way the site will have all of its “stuff” in one place, and the only global variable created was the MySite variable. If you’re creating a class that’s of general use, it’s somewhat permissible to place this in the global scope. The Prototype library, for instance, does this with classes such as Hash and Enumerable, while other libraries such as YUI create only one global variable which contains everything in it.

9) window.open

Except for a few cases, this method should really not be used. The most egregious misuse of this method is to open an entire website in a new window with no address bar, toolbar or scrollbars.

10) window.location, document.href, window.status

These, and I’m sure many other, properties are often misused. The location and document.href properties are most often used to redirect the user to a different page. While this is not really ideal in most cases, there are some situations where it can be useful, but they should be used sparingly. The window.status property is just wrong, and I’m not even sure if it works in modern browsers, but if you see this being used in a script you can be sure the script is outdated.

11) document.forms, document.all, document.layers

Like window.status, these properties are relics from the past which should not be used today. They were used as shortcuts to get to various parts of the document, but there are much better ways to do that nowadays. To get to an element in the DOM today, you should use the getElementById and DOM access methods and properties on elements such as element.childElements and element.parentNode. The most recent browsers even lets you access arbitrary elements using XPath or CSS. If you’re using a library such as Prototype or jQuery, these include some very intuitive and easy to use methods for walking the DOM tree.

12) setTimeout and setInterval using strings

The setTimeout and setInterval functions allow you to execute a piece of code at a later point or at a set interval. They can take either a string which is evaluated as code or a function which is run directly. The preferred option is to use a function.

//This is wrong
setTimeout("element.addClassName('foo');", 100);

//This is right
setTimeout(function(){
  element.addClassName("foo");
}, 100);

13) with

The with keyword allow you to execute a piece of code as if the global context was an arbitrary object:

var element = $('foo');
with (element) {
  title = "foo"; //element.title = "foo"
  getChildren().first().remove(); //element.getChildren.first().remove()
}

While this may save a few keystrokes it can cause more trouble than it’s worth. The most harmful side effect of using the with statement is the accidental creation of global variables, which we already know are evil. If you try to assign a property that the object (element from the example) doesn’t already have, this will evaluate to mean a variable name, which without the var keyword will be considered global.

14) document.write

Like window.open, this method is a relic from the web’s early days and should not be used. If you see this in a script, it’s likely that it’s a few years past its expiration date.

15) Not following naming conventions

The naming convention for variables and properties in JavaScript is as follows:

While not following these conventions is not going to break your script, it’s going to make it harder for others to read and understand. In particular, constructor functions (classes) should use an uppercase first letter to distinguish it from other functions. This is also the convention if your classes are properties of a namespace object, as they should be most of the time: new MySite.ClassName().

Posted by Tore Darell on Mar 14 2010

Comments

Log in to comment.