On Mar 13, 9:31 am, Jason Mulligan <[email protected]>
wrote:
> Hi,
>
> I just released abaaso 1.2 and I'd love some feedback on the global
> helper, aka $(). It's sole purpose in the lib is to retrieve Elements,
> it's not like jQuery; all the real code sits in a global namespace.
When posting code, please manually wrap it at about 70 characters and
use 2 or 4 spaces for indent. Allowing auto-wrapping means others have
to re-format your code before running it, which is not only
inconvenient but may introduce errors.
It should also be posted ready to run, so it can be copy and pasted.
>
> /**
> * Returns an instance or array of instances
The term "instance" doesn't have any meaning in ECMAScript or
javascript. The function returns (references to) either: a DOM
element, a NodeList, an array of elements, an array of NodeLists or a
mix of NodeLists and elements.
> *
> * @param arg {string} Comma delimited string of target element.id
> values
>From the code below, it might be class and tag names too.
> * @param nodelist {boolean} [Optional] True will return a NodeList
> (by reference) for tags & classes
There is no need to specify "by reference", that is understood in
javascript.
> * @returns {mixed} instances Instance or Array of Instances
There are no 'instances'. Perhaps you mean that multiple class or tag
names may return an array of NodeLists if nodelist is true, or array
of arrays if NodeList is false.
> */
> $ : function(arg, nodelist) {
"$" has been abused enough, function names should give some hint as to
what they do. Please give it a sensible name - getElements seems
suitable.
> try {
Why try..catch?
> arg = (arg.toString().indexOf(",") > -1) ?
> arg.split(",") :
> arg;
This is a very convoluted way of seeing if arg is a list or single
selector. Why not just assume it's a list and go fromt there:
arg = arg.split(/\s*,\s*/);
and remove troublesome whitespace between commas at the same time as
peole often like to write ('id, id, id') rather than ('id,id,id').
> nodelist = (nodelist === true) ? true : false;
> if (arg instanceof Array) {
If you turn the list into an array always then you don't need this
test. I'd rather see a typeof test than instanceof, so:
if (typeof arg != 'string') {
> var instances = [],
> i = arg.length;
>
> while (i--) {
> instances.push($(arg[i], nodelist));
> }
> return instances;
I don't understand why recursion is used. Just loop over the members
of the arg array (as the function is about to do). The extra function
calls just waste processor cycles (not that the difference in
performance is measurable, but there's no point in being wasteful).
Variable names should reflect what they represent, in this case
'instances' is an array of references to DOM elements. The name should
reflect that, say 'elements', 'result' or similar.
> }
>
> var obj;
> arg = new String(arg);
It isn't neccessary to create a String object here. You started with a
string, converted it to an array and now turn it into a String.
Simpler ways of turning an array into a comma separated string are:
arg = arg.join();
arg = '' + arg;
>
> switch (arg.charAt(0)) {
Ok, so that should be:
switch (arg[0])
Now I'm really confused - arg is changed from string to array to
String so it can be treated like an array.
> case ".":
You specified arg as a "Comma delimited string of target element.id
values", but it seems it does class name and tag name too.
> obj =
> document.getElementsByClassName(arg.substring(1));
You *must* feature test for document.getElementsByClassName, it's not
that widely supported.
> (nodelist === false) ? obj =
> Array.prototype.slice.call(obj) :
Ooops! Fail in IE. Use a toArray function.
Maybe that was the inspiration for try..catch? Far better to fix
errors if at all possible, try..catch is a last resort.
> void(0);
That is an abuse of the ternary operator as the false option is
effectively "do nothing". A more sensible approach would be:
obj = (nodelist)? obj : Array.prototype.slice.call(obj);
or more robustly:
obj = (nodelist)? obj : toArray(obj);
where toArray converts a NodeList or similar to an array:
function toArray(o) {
var a = [],
var i = o.length;
while (i--) {
a[i] = o[i];
}
return a;
}
In any case, an if statement is easier to read and maintain, less code
and (probably) faster:
if (!nodelist) obj = toArray(obj);
> break;
> case "#":
> obj =
> document.getElementById(arg.substring(1));
> break;
> default:
> obj = document.getElementsByTagName(arg);
> (nodelist === false) ? obj =
> Array.prototype.slice.call(obj) :
> void(0);
As above.
> break;
There is no need for break here.
> }
>
> obj = (obj === null) ? undefined : obj;
Don't do that, you are making debugging more difficult. The difference
between null and undefined can be useful to the caller.
> return obj;
> }
> catch (e) {
> error(e);
> return undefined;
That last statement is unncessary. If not given a return value, the
function will return undefined anyway.
> }
> }
I expected that when given ('#id,.class',true) I'd get back an array
with an element and a NodeList, but the order was reversed (NodeList,
element), which is unexpected.
Here's a similar function that is simpler and returns what I think the
above is supposed to return (note no feature testing so
getElementsByClassName will fail in browsers that don't support it):
/*
* Returns:
* a single element
* a single NodeList
* an array of elements
* an array of NodeLists
* an array of elements and NodeLists
*
* @param list {string} Comma separated list of ids, classes
* or tag names
* @param nodelist {boolean} [optional] If false, NodeLists
* are converted to arrays before being
* returned.
* @returns {mixed} Single element or NodeList, or an
* array of elements and NodeLists
*
*/
var getElements = (function() {
var re = /\s*,\s*/;
return function(list, nodelist) {
if (list.length == 0) return;
list = list.split(re);
var result = [];
var item, c, t, s;
for (var i=0, iLen=list.length; i<iLen; i++) {
item = list[i];
c = item.charAt(0);
s = item.substring(1);
// id
if (c == '#') {
t = document.getElementById(s);
} else {
// class
if (c == '.') {
t = document.getElementsByClassName(s);
} else {
// tag
t = document.getElementsByTagName(c + s);
}
// Turn nodelist into an array if wanted
if (!nodelist) t = toArray(t);
}
result.push (t);
}
// Return single member if only one
return (result.length == 1)? result[0] : result;
}
})();
--
Rob
--
To view archived discussions from the original JSMentors Mailman list:
http://www.mail-archive.com/[email protected]/
To search via a non-Google archive, visit here:
http://www.mail-archive.com/[email protected]/
To unsubscribe from this group, send email to
[email protected]
