Thanks for the critiques, I shall make the appropriate changes. On Mar 14, 11:39 pm, RobG <[email protected]> wrote: > 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]
