airbnb / javascript Goto Github PK

Double quotes used for strings in the last whitespace example

var leds = stage.selectAll('.led')
.data(data)
.enter().append("svg:svg")
.class('led', true)
.attr('width', (radius + margin) * 2)
.append("svg:g")
.attr("transform", "translate(" + (radius + margin) + "," + (radius + margin) + ")")
.call(tron.led);

I don't think this is bad, I really like this code style, it will lead me to less type error;

// bad
var once
  , upon
  , aTime;

I prefer this more

// bad
var once
  , upon
  , aTime
  ;

I disagree that setValue/getValue style accessors are better than jQuery-style value()/value(10) accessors. They add unnecessary name length, and require twice as many function definitions.

Admittedly, it's slightly faster for a VM to deal with functions that always return a single value type, as opposed to sometimes returning a type and sometimes returning nothing. But it's also slightly faster to download scripts using jQuery-style accessors, because the function keyword, prototype keyword, and public property names are unminifiable -- and we've yet to run into issues where our JS was too slow (whereas we do run into issues where our files are too big). If we have to compromise in one direction, I'd lean towards filesize over VM execution speed.

Additionally: the setters can still return a value, despite being "only" setters. Which means the VM executes just as quickly as it normally does.

It's also just less typing.

In the Type Casting & Coercion section, you've got the following:

// bad
var val = inputValue >> 0;

// good
/**
 * parseInt was the reason my code was slow.
 * Bitshifting the String to coerce it to a
 * Number made it a lot faster.
 */
var val = inputValue >> 0;

Can you clarify whether bitshifting is good or bad?

Every number is stored with 64bit, except you are doing bit shifting. Then javascript stores the number into a 32bit integer. The result of this: You get negative or just wrong numbers when you are shifting a big js-representing integer.
If you really think bit shift is a good way to improve performance, make sure the numbers are not bigger than 32 bit.

In the jQuery section, there is this convention:

Scope jQuery object queries with find. jsPerf

But the jsPerf cited is actually a different test than the example given. The example prefers $('.sidebar').find('ul') over $('.sidebar ul') and even $('.sidebar > ul') however the jsPerf test is not equivalent to the style guide example because it is essentially $(CACHED_NODE).find('ul') which means it doesn't need to find the first element.

If you take a look at this modified jsPerf here you will see that 'select and find' actually has nearly as bad as performance as the 'find in context of' style, 90% slower than 'find on cached node'. Cascade and 'parent > child' selector are only 50% and 40% respectively.

Essentially, the citation behind this convention is not applicable.

Are trailing commas good style in javascript?
I always use them for convenience.

var tom = {
  name: 'Tom',
  age: 20,
  friends: [
    'Jerry',
  ],
};

I believe there is mistype for ! sign recommendation on modules section. Actually I could not see how exclamation sign could help you in case missing semicolon. The following example will through error.

function abc() {} abc() !function bca() {}();

It would be much helpful to advice put ';' sign instead

function abc() {} abc() ;(function bca() {})();

and

function abc() {} abc(); ;(function bca() {})();

both equally safe for minification/concatenation.

Best

This is more of a personal preference than anything else. Of course, you're free to implement whatever you'd like, but declaring leading commas as unilaterally "bad" is disingenuous and misleading (no pun intended). Leading commas definitely have their pros as much as trailing commas have their cons.

We see two different IIFE styles in our codebase; do we have a preference?

// immediately-invoked function expression (IIFE)
(function() {
  console.log('Welcome to the Internet. Please follow me.');
})();

vs

// immediately-invoked function expression (IIFE)
!function() {
  console.log('Welcome to the Internet. Please follow me.');
}();

Note: allowed in ECMAScript 5.

There is a mistake under the accessors section.

Jedi.prototype.set: function(key, val) {
  this[key] = val;
};

Should read

Jedi.prototype.set = function(key, val) {
  this[key] = val;
};

I think that would be a good idea to create a .jshintrc config file to describe all these conventions.

In the Strings section the standard is:

"Strings longer than 80 characters should be written across multiple lines using string concatenation."

I understand the clarity of code - but the performance is 80-100% worse. jsPerf

The need for such long strings is debatable anyway, but isn't this kind of a steep performance penalty for code style?

We're already conforming to all of these standards, except for leading commas ;) Thanks for the great guide!

Please add us to your list! http://www.mightyspring.com

I see self, _this, that, and a whole bunch of other variable names used to save references to this. It would be nice to standardize on one of these.

I am partial to self but would be happy with agreement on anything that's vaguely reasonable (elephant is not reasonable, although it would be pretty funny to have elephants all over our codebase).

"Don't use reserved words as keys. It won't work in IE8."

//....
var superman = {
  default: { clark: 'kent' },
  private: true
};

This is stupid. Well it is not stupid in the context where you want to support IE8, but to sacrifice code quality because of inferior JS interpreter is stupid. The code is not wrong, IE8 is wrong for having dumb JS compiler. You shouldn't say that it is "good" for people to make sacrifices to their code to support stupid software. The good thing is that MS fixes IE. At least you should note this and mark this guideline as transient. Further IE8 is old even by today's standards and claiming such practice as good where nobody will need to consider IE8 very soon (or IMO nobody need to consider IE9 already but this is off topic) is not good.

BTW the Rhino JS engine is also dumb regarding property names so using such names causes problems with YUI compressor. But this is easily worked around by placing the property in quotes.

Maybe I'm wrong but: NaN == false is false.

The docs have this:

• Always return as early as possible

  // bad
  function() {
    var size;
  
    if (!arguments.length) {
      size = false;
    } else {
      size = arguments.length;
    }
  
    return size;
  }
  
  // good
  function() {
    if (!arguments.length) {
      return false;
    }
  
    return arguments.length;
  }
  
  However I've generally learned that this is a bad pattern because it can make the code harder to read as it gets more complicated.  Thoughts?

Could this:

(function() {
  console.log('Welcome to the Internet. Please follow me.');
})();

Be this? :

// Crockford's preference - parens on the inside
(function() {
  console.log('Welcome to the Internet. Please follow me.');
}());

I'm looking around the doku and find this example at the doku:
https://github.com/airbnb/javascript#properties

I think isJedi should be always a boolean. So, I would prefer to rename the method to hasProp() with typecasting at the return statement (return !!luke[prop];).

If you like to focus on this idea you can write:

var luke = {
  jedi: true,
  age: 28
};

function getProp(prop) {

  return luke[prop];
}

function hasProp(prop) {

  return !!getProp(prop);
}

var isJedi = hasProp('jedi');

Was reading your guidelines and actually noted that Javascript perform differently on all browsers. You suggest to use Array.apply(null, item) but on Safari 6, OS X, this method is 80% slower than Array.prototype.slice, was using the very same link to test: http://jsperf.com/converting-arguments-to-an-array/7

Can you please add style guide for Javascript closure. It will be very helpful.

Spotted a tiny thing. Not really important. Nice work on the guide btw :)

// good (slower)
$sidebar.find('ul');

// good (faster)
$($sidebar[0]).find('ul');

Not sure it's worth preferring such micro-optimizations, but FWIW the former is marginally faster.

new Object() is used for creating instance, where "Object" is a JavaScript class and not static object. Where as var object = {} is used to actually create an object. These are two different use cases. There is nothing good or bad about it.

I've googled. self is bad because of window.self.
But why did you choose _this over that?

I came across this issue today and thought it might be worth adding to the style guide:

When attaching data payloads to events (whether DOM events or something more proprietary like Backbone events), I think it's preferable to pass a hash instead of a raw value. For example, instead of:

$(this).trigger('listingUpdated', listing.id);

...

$(this).on('listingUpdated', function(e, listingId) {
  // do something with listingId
});

prefer:

$(this).trigger('listingUpdated', { listingId : listing.id });

...

$(this).on('listingUpdated', function(e, data) {
  // do something with data.listingId
});

Why? Well, in the second example, if one were to add more data to the event, one wouldn't need to find and update every handler for listingUpdated. It's similar to how it can be preferable to pass an options hash in Ruby instead of optional parameters.

If we like this, I can write it up and submit a pull request.

Edit: Added e as the first argument to the callback functions. Thanks @jamesreggio.

Personally, I think that it's better to use Tab button to indent code than two spaces.

First of all, its width is configurable by the coder. So if a coder likes 2-space indentation, he can set this in settings, if he likes 4-space indentation, he can set this in settings as well.

Second, tab is 1 character, so it takes 2 times less space than two spaces.

It's also quicker to add, remove or count indentation level with tabs. It's easy to get lost while removing a level of indentation from a deeply indented block unless you have a special editor configured to remove 2 spaces at once. With tabs, you don't have to divide the number of spaces by 2 to get current indentation level, and it's faster to navigate in indentation with arrow keys.

And, 2-space indentation is barely convenient. Its width is even less than line height.

Contrary to popular belief, using tabs to indent does not make the code look broken in different editors (except for Microsoft Word, but nobody codes in it). You just shouldn't use tab inside or after a line of code.

This article explains how to use tabs correctly: http://blogs.msdn.com/b/cyrusn/archive/2004/09/14/229474.aspx

Under Naming Conventions, use camelCase you have the example:

var user = new User({
  name: 'Bob Parr'
});

But you say not to use new for object creation (which I agree with).

You then continue with Use PascalCase when naming constructors or classes

This seems a little contradictory.

The possibility of showing relationships with indents for jQuery object chaining is something that might be worth considering for a style guide.

Blocks at even indent level are all acting on the last selector in the chain, this can help visualize some confusing subselectors. The trailing semicolon might be considered slightly idiosyncratic, but can be thought of similar to a closing bracket.

$('#foo')
  .find('.baz')
    .doSomething()
    .end()
  .find('.moo')
    .doSomethingElse()
;

It would be good if https://github.com/polarmobile/coffeescript-style-guide#annotations will be added to repo or with some changes ? What do you think ?

Hi there,

I was surprised to see that Array.push was so much slower than direct assignment. The JSperf link you give looks like a good proof of this.

However, the latest revision of the same bench, which has much cleaner code, seems to remove this difference, and even show push as faster in recent Chrome versions (which I would have spontaneously thought, considering the JIT optimizations that can be done on predictable function calls rather than guessing numerical access).

Are you sure that this recommendation is still to be followed?

All the best,
Matti

• JSbooks is a list of free online JavaScript books.
• Possibly also interesting – I’ve written a brief intro to JavaScript: “Basic JavaScript for the impatient programmer”.

I wouldn't call this "good".

var age = 0;
//...
var hasAge = !!age;

If you think about it there is no way to cast number to truth. All numbers are equally true, they are just numbers. The logical negation operator converts from numbers to bools but this is just a dumb legacy from C where there are no real bools and convenience but has nothing to do with good practice. And further - double negation a good practice - are you serious? This damages readability so much, it could really confuse young people making them wonder if this is some kind of new operator and even experienced developers will need to think about what it does. And if you are debugging and you are in some kind of confusion already this will only add to the confusion. Then you need to be aware of all language specifics like what negation does to strings, empty strings, arrays, empty arrays, objects, empty objects, bools, numbers, specific numbers. If it was strongly typed language like C it is better because you know the type of "age", but in JS, and particularly in debugging/undocumented/confused situation you don't know the type of age and the code will produce different results.

The right thing to do would be.

// bad.. just think about it - what does "not age" even means?
// further the intention is not clear
if ( !age ) {}

// good, the intention is perfectly clear
// further the code will work even with wrong types
if ( age > 0 ) {}
if ( age !== 0 ) {}

// good
var hasAge = (age > 0);

It's a small thing, but we should probably be consistent here, too. What's the best way to cast to integers? I have the habit of bitshifting by 0 "15" >> 0; it's one of the more performant ways to do it, and it's concise, but it also isn't super obvious.

Under the section about PascalCase in Naming Conventions is the block

// good
function User(options){
  this.name = options.name;
}

As per the Whitespace section, there should be a space between the closing parens and the brace on the function definition.

Hi,

about the "jQuery Find vs Context, Selector" test linked in the performance section.
http://jsperf.com/jquery-find-vs-context-sel/13

I just took the test @ jsPerf, expecting a somewhat similar result as find/context internally end up using the same method (if I remember correctly).

But the test is misleading, comparing 2 uneven tests.

$article = jQuery object containing 10 article nodes
article = only one single article node

So the only things you can compare in the test are:

• "find method (node context)" (48,378) vs "context node" (46,031)
• "find method (jquery context)" (6,679) vs "context jQuery" (7,030)

"context jQuery[0]" is basically the same as "context node"

TL;DR
The linked test is misleading as it's comparing different things, so let's either add a new revision with an accurate comparison or just remove the link?!

BTW, we're about 95% compliant to your style guide, trying now to get to 100%. Great work from you guys!

Under Objects it says you shouldn't use reserved words. I'm not sure this is important, though avoiding them might make sense for readability. At least using klass instead of class would seem wrong to me, as you are corrupting a word instead of finding a readable synonym (like type).

Right now you can use just about anything. E.g., test = {function: 1}; test.function === 1 –but additionally I think we can be confident this will continue to be the case. IndexedDB uses the method cursor.continue() which isn't just a future-protected reserved word (like class) but a reserved word right now. But IndexedDB represents current API design by people involved in the standardization process.

I think it's questionable from a readability point of view to use current keywords, but I don't see any reason at all to avoid words reserved for possible future use (class and private, as used in the example, are both examples of that).

Just a small comment.

Bitshifting seems to be really fast on Firefox, (18) but- for as far as I can see much slower on Chrome (24)

http://jsperf.com/bitshifting-or-typecasting

First off this is a great document, and thanks for sharing it. I have read the entire file end-to-end as I work on my own personal style guide. However, I was looking at your module example and you assert that "!" protects you from concatenation errors. However, I don't think this is correct. Consider the following example:

// => SyntaxError: Unexpected token !
!function(){console.log("ham");}()!function(){console.log("cheese")}();

Maybe I am misunderstanding how you expected things to work.

I think you still need to prepend a semicolon to truly protect yourself from concatenation errors.

;!function(){console.log("ham");}()

Your "good" string example. Could be optimized.
First of all you are using items[i] = ... and in another example you point this is "bad" and say the "good" is to use items.push(). Then since you already know the size of the array you could pre-allocate with items = new Array( length );. Although I must point that this may actually degrade performance in some cases where JS engines are optimized for "bad" practices and when you do the right thing you may actually break the optimization.

// good
function inbox(messages) {
  items = [];

  for (i = 0; i < length; i++) {
    items[i] = messages[i].message;
  }

  return '<ul><li>' + items.join('</li><li>') + '</li></ul>';
}

Was reading through the style guide, but couldn't find anything about this. Instead of

var callback;

if (callback) {
    callback();
}

I've started using

var callback;

callback && callback();

Which I haven't found any issues with. Would this style guide frown upon this?

It would be nice if each rule had a reason why one style was preferred over another. i.e. performance, js good parts reference etc.

i.e. instead of just
Don't do X
state:
Don't do X because Y and Z

I find people treat undefined incorrectly a lot. It's a value, you can use it, test for it, etc, but people frequently use typeof x == "undefined" when they don't need to.

I made a fork with a big changeset of my own opinions, but here's the section I wrote about undefined:

## <a name='undefined'>undefined</a>

  - `undefined` is an object.  You should use it as an object.  You can test for it.  For instance:

    ```javascript
    // good
    function pow(a, b) {
      if (b === undefined) {
        b = 1;
      }
      return Math.pow(a, b);
    }

    // bad
    function pow(a, b) {
      if (typeof b == "undefined") {
        b = 1;
      }
      return Math.pow(a, b);
    }
    ```

  - *Only* use `typeof x == "undefined"` when the variable (`x`) may not be declared, and it would be an error to test `x === undefined`:

    ```javascript
    if (typeof Module == "undefined") {
      Module = {};
    }

    // But also okay, for browser-only code:
    if (window.Module === undefined) {
      Module = {};
    }
    ```

    Note that you can't use `window` in Node.js; if you think your code could be used in a server context you should use the first form.

I personally use t to store my reference to this.

I understand that self resembles PHP or PERL, but it is not the same thing. It's bad.
that is even worse...

...but why _this? It looks similar to "private" variables. It's at least confusing...

I'm puzzled in here :) Plese explain.

From Crockford:

If a function literal is anonymous, there should be one space between the word function and the ( (left parenthesis). If the space is omited, then it can appear that the function's name is function, which is an incorrect reading.

div.onclick = function (e) {
  return false;
};

that = {
  method: function () {
    return this.datum;
  },
  datum: 0
};

Source: Code Conventions for the JavaScript Programming Language

I think it would be useful to provide recommended jsHint config file for this StyleGuide. Something like this.

Using logical NOT seems faster on Chrome:

http://jsperf.com/js-convert-boolean/2

though it comes with a loss of code readability.

One of my favorite parts about the JavaScript community is that people choose to write it in so many different ways.

Why do you want to ruin that with a style guide?

Imagine if when Picasso was learning to paint, we told him what style he should paint in. Would we have cubism?