All your points are well taken, and a better analysis of the code by far than my hasty reaction.
So what was bothering me about the first example? Probably the repetition of the indexOf() tests, combined with one of the indexOf() tests being >= 1 and the rest >= 0.
But you're right, it's not nearly as bad as I made it out to be.
Since I've put my foot in my mouth, I guess I'll put my money there too and show how I might have done it. If I were doing UA detection at all, that is:
But that fails on one of your points, since it returns an object instead of setting properties of 'this'. It's also less flexible - what if one of the tests needed more than a simple string comparison? At least it's simpler?
So who am I to criticize? :-)
On the second example, it's not just that function - the entire web page is full of similar code. Here's another snippet:
addressCheckMsg="";
if(!type)
{
iLen = line1.value.length;
for(i=0; (i<4) && (i<iLen); i++)
{
var ch = line1.value.substring(0,i+3);
chUpper=ch.toUpperCase();
switch(chUpper)
{
case 'PO BOX':
addressCheckMsg += " Resident address can not be a P.O. Box.\n";
i=iLen;
break;
case 'P.O. BOX':
addressCheckMsg += " Resident address can not be a P.O. Box.\n";
i=iLen;
break;
case 'P. O. BOX':
addressCheckMsg += " Resident address can not be a P.O. Box.\n";
i=iLen;
break;
case 'P O BOX':
addressCheckMsg += " Resident address can not be a P.O. Box.\n";
i=iLen;
break;
case 'POB':
addressCheckMsg += " Resident address can not be a P.O. Box.\n";
i=iLen;
break;
default:
break;
}
}
}
Yikes. I'd better not say more or I'll start foaming again... :-)
Your first example has nice trickery in it but I prefer the original version. I think it is important to keep it simple.
Compressed code is often not the best way to do it; adding a few lines of verbosity can reduce the time it takes to understand the code to a fraction while sacrificing very little in terms of performance.
It's good that this discussion can actually be had on here civilly. Too often I see vitriol and pedantic disagreement on HN for no reason other than the swinging of the e-peen.
All your points are well taken, and a better analysis of the code by far than my hasty reaction.
So what was bothering me about the first example? Probably the repetition of the indexOf() tests, combined with one of the indexOf() tests being >= 1 and the rest >= 0.
But you're right, it's not nearly as bad as I made it out to be.
Since I've put my foot in my mouth, I guess I'll put my money there too and show how I might have done it. If I were doing UA detection at all, that is:
But that fails on one of your points, since it returns an object instead of setting properties of 'this'. It's also less flexible - what if one of the tests needed more than a simple string comparison? At least it's simpler?So who am I to criticize? :-)
On the second example, it's not just that function - the entire web page is full of similar code. Here's another snippet:
Yikes. I'd better not say more or I'll start foaming again... :-)