Hacker News new | comments | show | ask | jobs | submit login

I was foaming at the mouth a bit, wasn't I?

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... :-)



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.

-----


I disagree about the first one. I think that is easier to understand, easier to add more options and less prone to errors.

-----


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.

-----




Applications are open for YC Summer 2016

Guidelines | FAQ | Support | API | Security | Lists | Bookmarklet | DMCA | Apply to YC | Contact

Search: