Hi Rob,
I made a few of the changes you suggested, and I disregarded some. I'm
of the opinion that try/catch is essential for proper applications;
where exceptions are valid and expected (rbac for example).
Considering it's simply another object being generated; and JS engines
& the hardware running them get faster I don't subscribe to the mantra
from the 90s/00s.
Silent failures are good.
Thanks for bringing the modulus operator to my attention! It's much
better than what I had, even if I had just changed it to a
parseFloat(). I wrote mine a little differently:
even : function(arg) {
try {
if (typeof(arg) != "number") {
throw label.error.expectedNumber;
}
return ((arg % 2) === 0) ? true : false;
}
catch (e) {
error(e);
return undefined;
}
}
I was thinking about your comment on how I'm executing init(), and in
the end every library is doing client specific methods. I'm just
keeping it easy to understand.
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]
