minitech/Sprint.js — Issue #3: Some feedback
Author: alexanderdickson
Created: 2011-10-31 12:41:46+00:00
State: open

Hi Minitech.

Good job on the selector engine.

Just a few thoughts (let me know if you'd like me to contribute these).

The code isn't commented enough. What does `combine()` do exactly? I should be able to read a one line comment and know what it does without mentally stepping through the code.

If you want to check for a valid `id` attribute, check for the first char as `[a-zA-Z]` as a leading number is an invalid `id` attribute. I'm not sure if this actually chokes any browsers though.

`$unescape` uses a callback with `replace()` when `'$1'` would probably be a better fit.

`instanceof Array` (like checking the `constructor`) doesn't work in a multi `window` environment, such as `iframe`s. You're better off using `Object.prototype.toString.call(someVar) === '[object Array]'`.

It would be worthwhile caching the `length` property when iterating over an array. It makes a small performance improvement, most notable in older IEs.

`combine()` defines `i` twice. It should only define it once.

`combine()` takes two arrays. It might be beneficial to take an arbitrary number of arrays. Use the `arguments` object.

If `toArray()` is to convert an Array like Object to a real Array, you could simply use `Array.prototype.slice.call(obj)`.

Multiple `var` definitions should be condensed to one `var`, it's easier to read IMO.

`typeof` can be fishy when passing object representations of primitives. Again, `Object.prototype.toString.call()` is your friend.

Shimming `getElementsByClassName()` can be dangerous, as the native method returns a live NodeList, whilst it is impossible/difficult to shim that exact functionality.

Why are you pushing string representations of functions to `criteria`. Could you refactor them to anonymous functions being passed which do the filtering?

You should store where `$` originally points to, and provide a mechanism to disable assigning it to `Sprint`.

That's a quick look over, hopefully you'll find this beneficial. :)