JavaScript: Bad Practices

I’ve seen a lot of curious (bordering on horrific) code in my life; and I’d say about half of it was written by me. If you don’t attest to the fact that you once wrote crap code then you’re either a liar or perhaps, have omnipotent powers!

Here’s a relatively small collection of what I see as bad JavaScript practices; in other words – a list of how to write really really ugly JavaScript that will break! I know it’s quite a negative angle to take but it’s easier than writing a post on best practices, plus it gives me a chance to vent! Many of these are things I’ve done in the past; I’ve since discovered the errors in my ways.

Non-constructor identifiers beginning with a capital letter

Variable names should only begin with a capital if they’re pointing to constructor functions. Doug Crockford has more:

There is a convention that all constructor functions are named with an initial capital, and that nothing else is spelled with an initial capital. This gives us a prayer that visual inspection can find a missing new.

Not following conventions will only get you in trouble! So, to be clear, this is all wrong:

var Color = 'red';
var SomeNumber = 234;
var AnArray = [1,2,3];
var Foo = { bar: 123 };
// None of them should start with a capital letter!

Only functions that are constructors should be identified with an initial captial letter, not regular functions. E.g.

/* This is a regular function. */
function sum() {
    var total = 0, i = 0, len = arguments.length;
    for ( ; i < len; ++i ) {
        total += arguments[i];
    }
    return total;
}
 
/* This is a constructor function */
function Widget(innerText, styles) {
    this.innerText = innerText;
    this.cssStyles = styles;
}

Not using the var statement to define variables

Not using the var statement when defining variables for the first time is a very bad idea. These variables will become part of the global scope. This sucks because you won’t get any warning. Not only is it a bad practice but it can have a negative affect on performance, after-all, the further away a scope is the longer it takes to access. So, always use the var statement:

foo = 3; // NO!
var foo = 3; // YES

Prefixing every new variable in a given scope with var

There’s no point in having several var statements if you only need one:

var someVar1 = 'a';
var someVar2 = 'b';
var someVar3 = 'c';
 
// Much easier:
var someVar1 = 'a',
    someVar2 = 'b',
    someVar3 = 'c';

Using non-strict comparison operators, and then comparing across different types

There are two classes of comparison operators; those that type-cast (==/!=) when required and those that don’t (===/!==). You don’t ever want to use the former!

1 == "1"; // true
false == " nt "; // true
[[],[]] == true; // true
 
// ... Confused?

See? It’s obviously a bad idea! Please only ever use strict-equality comparison operators; they’ll check that the two operands are identical, not just “equal”:

1 === "1"; // false
false === " nt "; // false
[[],[]] === true; // false
 
// Just as expected!

Not “caching” non-varying complex objects

This is vitally important; if you’re going to be repeatedly using an object you should name it and save it! A basic example:

function myFunction(arg) {
 
    var configObj = {
        option1: 123,
        option2: 456
    };
 
    // do stuff
 
}

configObj is being created every time the function is executed. It doesn’t change on each call so there’s no point in having it in the function body. Here’s a better alternative:

var myFunction = (function(){
 
    var configObj = {
        option1: 123,
        option2: 456
    };
 
    return function(arg) {
        // do stuff
    };
 
})();

Here we’re storing configObj in a “private” scope only accessible to “child” scopes. It’s only being created one time so we’ve instantly shaved notable milliseconds off execution time!

Doing too much in a loop or recursive function

Doing too much in a loop can have terrible affects on performance. Every single thing you do inside a loop will add to its overall execution time and if this grows beyond about ~50ms (100ms max) the user will start noticing. There are a couple of small enhancements you can make to optimise your loops:

  • Cache object properties before the loop: 
    for (var i = 0, len = array.length; i < len; ++i) {}
/* Notice we're caching the "length" property
   so we don't have to check it on every iteration */
  • Loop in reverse if possible: 
    var i = array.length;
while (i--) {
    /* Combining the control condition and any control
       variable changes makes it much faster! */
}

Not understanding and therefore not appreciating the benefits in abstracting repeatedly utilized code versus having a wall of unreadable code

Urm, yeh, the title says it all really! Compare this:

document.getElementById('foo').style.border = '3px solid #FFF';
document.getElementById('foobar').style.border = '3px solid #FFF';
document.getElementById('foofoobar').style.border = '3px solid #FFF';
document.getElementById('foofoobarbar').style.border = '3px solid #FFF';

… to this:

var allMyFoos = '#foo, #foobar, #foofoobar, #foofoobarbar';
jQuery(allMyFoos).css('border', '3px solid #FFF');

(Yes, jQuery is a superb example of “abstracting repeatedly utilized code”)

Choosing terseness over readability

Readability is always better than terseness. There may be some situations where the more concise approach has other benefits but your main focus should normally be the readability of your code; not just to you, but to other developers. There’s no need to dumb-down your code though; readability is not the same as simplicity.

// 1:
while (parent.nodeName.toLowerCase() !== 'div' && (p = parent.parentNode));
 
// 2:
do {
    if (parent.nodeName.toLowerCase() === 'div') {
        break;
    }
} while (parent = parent.parentNode);
 
// 3:
do if (parent.nodeName.toLowerCase() === 'div') break; while (parent = parent.parentNode);

The above constructs all do the same thing but one is clearly more readable that the other two. Unfortunately with readability you gain (arguably) unnecessary cruft.

Not knowing what DRY is, or how to apply it to JavaScript

There are many different ways to express this concept but essentially it centers around not repeating yourself. A good example:

elemCollection[i].style.color = 'red';
elemCollection[i].style.backgroundColor = 'blue';
elemCollection[i].style.border = '2px solid #000';
elemCollection[i].style.paddingLeft = '3px';
elemCollection[i].style.marginTop = '3px';
elemCollection[i].style.fontSize = '1.2em';
elemCollection[i].style.fontStyle = 'italic';

The above mess can be expressed in a much cleaner way:

applyCSS(elemCollection[i], {
    color: 'red',
    backgroundColor: 'blue',
    border: '2px solid #000',
    paddingLeft: '3px',
    marginTop: '3px',
    fontSize: '1.2em',
    fontStyle: 'italic'
});

(Using the following helper function:)

function applyCSS(el, styles) {
    for (var prop in styles) {
        if (!styles.hasOwnProperty || styles.hasOwnProperty(prop)) {
            el.style[prop] = styles[prop];
        }
    }
    return el;
}

Commenting every line

As Jeff Atwood boldly said:

If your feel your code is too complex to understand without comments, your code is probably just bad. Rewrite it until it doesn’t need comments any more. If, at the end of that effort, you still feel comments are necessary, then by all means, add comments. Carefully.

We’ve all done it; commented almost every other line thinking that it brings a whole new level of clarity to our code, when it actually impedes readability and distracts from the code itself. Just try to limit how many comments you add; they’re not always as helpful as you think they are!

Using browser detection instead of feature detection

Feature-detection is future-proof. Browser-detection isn’t. It’s as simple as that!

If, for example, you needed to find out whether a browser supports the min-height CSS property, you could test it in the following way:

var minHeightSupport = (function(){
 
    var aHeight, bHeight,
        doc = document,
        aDiv = document.createElement('div'),
        bDiv = document.createElement('div');
 
    aDiv.style.position = bDiv.style.position = 'absolute';
 
    doc.body.appendChild(aDiv);
    doc.body.appendChild(bDiv);
 
    bHeight = bDiv.clientHeight;
    doc.body.removeChild(bDiv);
 
    aDiv.style.minHeight = (bHeight+1) + 'px';
 
    aHeight = aDiv.clientHeight;
 
    doc.body.removeChild(aDiv);
 
    return aHeight > bHeight;
 
})();

Writing feature-tests can be a long and challenging exercise but in the end you should be left with something that’s very reliable! The above test goes something like this:

  1. Create two DIV elements, “aDiv” and “bDiv”.
  2. Absolutely position them so the page doesn’t shift.
  3. Append them both to the document.
  4. Save bDiv‘s current height then remove it from the document.
  5. Set aDiv‘s min-height property to one more than bDiv‘s height.
  6. If aDiv‘s overall height is greater than bDiv‘s height then min-height must be supported!

(The above function is just an example; I haven’t tested it sufficiently yet.)

Creating DOM elements within a loop

It’s slow; very slow actually. When you have no other choice then make sure you’re using a document fragment that will be inserted into the document later, don’t create and append individual DOM elements in to the actual document.

Regular slow approach:

for (var i = 0; i < 100; ++i) {
    elementInDocument.appendChild(document.createElement('div'));
}

Using a document fragment:

var fragment = document.createDocumentFragment();
for (var i = 0; i < 100; ++i) {
    fragment.appendChild(document.createElement('div'));
}
elementInDocument.appendChild(fragment);

Using innerHTML and array.join() (even faster):

elementInDocument.innerHTML += Array(101).join('<div/>');

Using inline event handlers, YUCK!

Just don’t do it! Take the unobtrusive approach instead.

Big no-no! –

<a href="javascript:void doSomething();">Click</a>
<!--OR-->
<a href="#" onclick="return doSomething();">Click</a>

Much better:

jQuery('element').click(doSomething);

(I’m not saying you have to use jQuery but you should develop a couple of abstractions to make it easier to work with events and DOM elements – at the least)

Having long HTML strings in your JavaScript

They’re ugly and hard to maintain. Either use DOM methods or put the HTML elsewhere; maybe in a template file or somewhere in the document, in a comment node or a hidden element. Seriously, just look at this:

var widgetStructure = '<div id=' + widgetID + '>' +
                           '<h2 id="' + widgetTitleID + '">' + widgetTitle + '</h2>' +
                            '<div id="' + widgetContentID + '" style="' + widgetContentStyles + '">' +
                                (function() {
                                    var paras = '';
                                    for (var i = 0; i < paraItems.length; ++i) {
                                        paras += '<p>' + paraItems[i] + '</p>';
                                    }
                                    return paras;
                                })() +
                            '</div>' +
                        '</div>';

Yes, I have seen that done before; ugly as sin! Please don’t do it!

Thanks for reading! Please share your thoughts with me on Twitter. Have a great day!