Hacker Newsnew | comments | ask | jobs | submitlogin
gcr 383 days ago | link | parent

> This is code from someone who has no idea how to program

That's a quite strong assertion. What's wrong with your first example? I can think of very few criticisms (s isn't needed for example) but there's lots of things they did well:

- It follows the best practices for an OO constructor (doesn't return the object, just sets properties of `this`)

- All temporary variables are local. No global pollution (besides the "Browser" function itself, but because you're quoting it out of contect, I can't tell if even that's local or not)

- Degrades gracefully (everything is null) instead of picking a default incorrect choice

Sure, I would have written it differently, but so would everyone else here.

As for your second example, sure, it's not great, but I can sort of imagine some sleep-deprived developer coding up that to interop with some auto-generated DOM elements from an old PHP script left behind by a forgotten intern. We need more context here.



Stratoscope 383 days ago | link

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

-----

fubari13 383 days ago | link

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.

-----

devgutt 382 days ago | link

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

-----

d23 382 days ago | link

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.

-----

buremba 383 days ago | link

actually in this code, "this" refers to the window object. it means it's same as window.version.

-----

Stratoscope 383 days ago | link

I thought that for a moment, but it isn't so. They do call the Browser() function as a constructor with 'new Browser()', so 'this' is the object it's constructing.

-----




Lists | RSS | Bookmarklet | Guidelines | FAQ | DMCA | News News | Feature Requests | Bugs | Y Combinator | Apply | Library

Search: