On Mar 14, 8:43 pm, Jason Mulligan <[email protected]>
wrote:
> Hi Rob, thanks for looking. It would've been better if you had looked
> at the lib prior to making your comments, for context.
Ok, checked out the lib. I don't have time to go over all of it, a
couple of quick comments:
if (typeof(document.getElementsByClassName) != "function") {
typeof is not guaranteed to work on host objects the same as it does
for native objects (and is known not to in at least one widely used
browser). In IE:
alert(typeof document.getElementById);
shows "object". Considering the above test is supposed to be run in
IE, I think you've not tested enough, it "works" by accident.
<URL: http://groups.google.com/group/comp.lang.javascript/msg/197a9ad02f01e47c
>
The library seems dependent on browser sniffing, mostly IE but others
too. That is a bad strategy for a script library. Consider the
following:
// Registering events
if ((abaaso.client.chrome) || (abaaso.client.firefox) ||
(abaaso.client.safari))
window.addEventListener("DOMContentLoaded", function(){
Other libraries use feature detection instead of browser sniffing,
check one out.
There seems to be an excessive use of try..catch, which you didn't
justify in the $ function when asked. For example:
even : function(arg) {
try {
return (((parseInt(arg) / 2).toString().indexOf(".")) > -1) ?
false : true;
}
catch (e) {
error(e);
return undefined;
}
},
Not only is try..catch completely unnecessary (and the function
returns incorrect results, e.g. even(12.3) returns true), but a vastly
simpler and more robust function is:
function isEven(n) {
n = Number(n);
return n == 0 || (n && !(n%2));
}
And it's isOdd partner:
function isOdd(n) {
n = isEven(n);
return typeof n == 'boolean'? !n : n;
}
So if n is any kind of number (e.g. 2.2e3) you will get the correct
result, and if it's not a number, both functions return NaN, which
evaluates to false, but also tells the caller that the supplied value
wasn't a number, which is useful if it cares to look.
For various validation regular expressions, try:
<URL: http://www.merlyn.demon.co.uk/js-valid.htm >
Minor quibble, but more ternary operator abuse:
((!client.ie) && (console)) ? console.error(err.message) : void(0);
Ok, I mentioned the guard pattern in jest, here's how you missed your
chance at hero worship :-)
!client.ie && console && console.error(err.message);
There is no need for the grouping or ternary operators, the following
is more easily understood by most programmers:
if (!client.ie && console) console.error(err.message);
Look ma, less code! ;-p
Oh, and the library does indeed add a getElementsByClassName property
to document - aAugmenting host objects is a bad idea.
The regular expression used in the className test is not optimal:
pattern = new RegExp("\\b"+arg+"\\b");
better to use the widely adopted:
pattern = new RegExp("(^|\\s)"+arg+"(\\s|$)");
That's it, I'm done. :-)
--
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]
