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:
function Browser() {
function is( ua, result ) {
var start = navigator.userAgent.indexOf( ua );
if( start < 0 ) return false;
result.version = result.version ||
parseFloat( navigator.userAgent.substr( start + ua.length ) );
return result;
}
return(
is( 'MSIE', { isIE: true } ) ||
is( 'Netscape6/', { isNS: true } ) ||
is( 'Gecko', { isNS: true, version: 6.1 } ) ||
{}
);
}
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... :-)