Hi Rob, thanks for looking. It would've been better if you had looked
at the lib prior to making your comments, for context.
The function returns either a NodeList passed by reference, or an
instance. An instance is an occurrence or a copy of an object, so it
should be a valid term; hence you get an instance of the NodeList if
it's not by reference. That's one reason I won't choose "elements",
since it's instances of elements.
I don't plan to reformat my code for this limited thread system; if
anything i'd rather be that guy that eventually causes an admin to
rethink their formatting limitations.
Picking at the docblock is interesting, it's changed since I posted.
Again, would've been useful to look at the entire picture. Also,
assuming that someone knows what's by reference and what isn't, is a
mistake in my opinion.
I was thinking about the split op and you have a point about the
spaces. I'll probably be making that change sometime this week. As for
the use of $(), why re-invent the wheel? People expect $() today and
even write their own simple versions. As an example, it's done
multiple times in the JavaScript Patterns book, for code samples on
other topics.
Arrays are objects, so a typeof() is not a valid test.
Recursions allows you get a 2d array within a 2d array. It's a good
pattern, one I reuse through the lib encase $() returns an array to
other methods.
There is no Array > String > String op occurring. You either have an
Array that's processed as one, and returns; or you have a potential
string argument that's cast because it's cast implicitly with the
following op; putting it up front shows the person what's occurring
and hopefully they'd understand why with the charAt(). I tried using
the Array index as [0], but IE doesn't support that (sad face).
The feature testing comments are out of place; look at the lib. The
init() sets up what's missing for IE. Array.prototype.slice() is from
1.5, which IE6 supports.
You need to learn how to love the ternary as an If-Else, it doesn't
have to set a value. What I did is legal, hence no hating it.
Now, the real input I was after... Why do you think iteration is
better than casting with slice()? That seems like true wasted cycles
to me, and I was hoping for some deep thought on it. Since slice is
just an expression; I haven't had time to dig to see what it's really
doing. Is it a simple 1 liner to the same iteration?
There's no reason why a switch can't be compiled to be faster than an
if-else, look at PHP's switch(true){} .. complete 180 in performance
from 4.0 to 5.2+
I do need to change that countdown, it's not valid for this use-
case ... that's a habit from other types of ops.
Thanks for looking.
On Mar 13, 11:34 pm, RobG <[email protected]> wrote:
> 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]
