minitech/Sprint2 — Issue #1: Some more feedback.
Author: alexanderdickson
Created: 2011-11-21 01:00:07+00:00
State: open

Congratulations on the new version.

Here is some feedback :)

###[Multiple `var` statements](https://github.com/minitech/Sprint2/blob/master/sprint.js#L8).

These should probably be condensed into one `var`. If anything, it's less noise and will give better minification if your minifier doesn't already optimise this.

###[`toArray()`](https://github.com/minitech/Sprint2/blob/master/sprint.js#L25) function.

Instead of creating a new `Array` each time, you could access `Array.prototype` or create the `Array` once somewhere. Also, the default start of `slice()` is `0`, so you could skip explicitly passing it.

###[Multiple assignments to the same value](https://github.com/minitech/Sprint2/blob/master/sprint.js#L148).

When dealing with primitives such as `false`, you can assign them all to one `false`, from right to left. This again will help with minification.

###[`switch` fall through](https://github.com/minitech/Sprint2/blob/master/sprint.js#L360).

These fall throughs may be considered *too clever*. You may want to make your comment more explicit.

As an aside, you could also do `type = ['[', ':', '.', '#'].indexOf(c)` or similar with an `Object`, but it will probably be slower.

###[Modulus comparison](https://github.com/minitech/Sprint2/blob/master/sprint.js#L395).

You should be able to just use `! (index % 2)`.

###[Selectors optimisation](https://github.com/minitech/Sprint2/blob/master/sprint.js#L492).

You could do the same thing with `head` (`document.getElementsByTagName('head'])[0]`) and `html` (`document.documentElement`) element.

###[Checking for child nodes](https://github.com/minitech/Sprint2/blob/master/sprint.js#L573).

There is [`node.hasChildNodes()`](https://developer.mozilla.org/en/DOM/Node.hasChildNodes).

###[Accessing `arguments`](https://github.com/minitech/Sprint2/blob/master/sprint.js#L580).

If you are just testing the `arguments.length`, you could check for `typeof newContents == 'undefined'`. Accessing `arguments` can be slower in older browsers.

###[Checking type for `String`](https://github.com/minitech/Sprint2/blob/master/sprint.js#L619).

Be aware this won't work if passed an instantiated `String` object. You could check the same way as you checked for the `Array`.

###[Convenience global](https://github.com/minitech/Sprint2/blob/master/sprint.js#L619).

You should just be able to use `$ = Sprint`.

I hope you find this helpful :)