Hacker News Reader

Comment by kevincennis | Hacker News Reader

There are a lot of things in this code that make me think that it was written by someone for whom JavaScript is not their main language - but probably the most glaring example is the use of `new Object()`. I've never seen anyone with more than 3 days JS experience use the Object constructor over a literal.

View on HN

Oh, it's just awful. It's worse than just "not knowing JavaScript." This is code from someone who has no idea how to program:

    function Browser() {
        var ua, s, i;
        this.isIE = false;
        this.isNS = false;
        this.version = null;
        ua = navigator.userAgent;
        s = "MSIE";
        if ((i = ua.indexOf(s)) >= 1) {
            this.isIE = true;
            this.version = parseFloat(ua.substr(i + s.length));
            return;
        }
        s = "Netscape6/";
        if ((i = ua.indexOf(s)) >= 0) {
            this.isNS = true;
            this.version = parseFloat(ua.substr(i + s.length));
            return;
        }
        s = "Gecko";
        if ((i = ua.indexOf(s)) >= 0) {
            this.isNS = true;
            this.version = 6.1;
            return;
        }
    }

But it's not just these people. Code like this is everywhere! Here's what I ran into on www.safeco.com today (NSFL!):

    function setupAddress(frm, i, clickevent) {
        if (frm["USERESADDASMAILINGMAIN" + i].checked) {
            if (frm.NEWRESIDENCEADDRESS1.value == "" && frm.NEWRESIDENCEADDRESS2.value == "") {
                alert("Resident address must be entered for this option.");
                frm.NEWRESIDENCEADDRESS1.focus();
                frm["USERESADDASMAILINGMAIN" + i].checked = false;
            }
            if (frm.NEWRESIDENCEADDRESS1.value == "" && frm.NEWRESIDENCEADDRESS2.value != "") {
                FieldSwap(document.frmMain.NEWRESIDENCEADDRESS1, document.frmMain.NEWRESIDENCEADDRESS2);
            }
            frm["NEWMAILINGADDRESS1" + i].value = frm.NEWRESIDENCEADDRESS1.value;
            frm["NEWMAILINGADDRESS1" + i].disabled = true;
            frm["NEWMAILINGADDRESS1" + i].onfocus = frm["NEWMAILINGADDRESS1" + i].blur;
            frm["NEWMAILINGADDRESS2" + i].value = frm.NEWRESIDENCEADDRESS2.value;
            frm["NEWMAILINGADDRESS2" + i].disabled = true;
            frm["NEWMAILINGADDRESS2" + i].onfocus = frm["NEWMAILINGADDRESS2" + i].blur;
            frm["NEWMAILINGCITY" + i].value = frm.NEWRESIDENCECITY.value;
            frm["NEWMAILINGCITY" + i].disabled = true;
            frm["NEWMAILINGCITY" + i].onfocus = frm["NEWMAILINGCITY" + i].blur;
            frm["NEWMAILINGSTATE" + i].value = frm.NEWRESIDENCESTATE.value;
            frm["NEWMAILINGSTATE" + i].disabled = true;
            frm["NEWMAILINGSTATE" + i].onfocus = frm["NEWMAILINGSTATE" + i].blur;
            frm["NEWMAILINGZIPCODE" + i].value = frm.NEWRESIDENCEZIPCODE.value;
            frm["NEWMAILINGZIPCODE" + i].disabled = true;
            frm["NEWMAILINGZIPCODE" + i].onfocus = frm["NEWMAILINGZIPCODE" + i].blur;
            if (i != 0) {
                frm["EXPLANATIONVEH" + i].value = "";
                frm["EXPLANATIONVEH" + i].disabled = true;
                frm["EXPLANATIONVEH" + i].onfocus = frm["EXPLANATIONVEH" + i].blur;
            }
        } else {
            frm["NEWMAILINGADDRESS1" + i].disabled = false;
            frm["NEWMAILINGADDRESS1" + i].onfocus = null;
            frm["NEWMAILINGADDRESS2" + i].disabled = false;
            frm["NEWMAILINGADDRESS2" + i].onfocus = null;
            frm["NEWMAILINGCITY" + i].disabled = false;
            frm["NEWMAILINGCITY" + i].onfocus = null;
            frm["NEWMAILINGSTATE" + i].disabled = false;
            frm["NEWMAILINGSTATE" + i].onfocus = null;
            frm["NEWMAILINGZIPCODE" + i].disabled = false;
            frm["NEWMAILINGZIPCODE" + i].onfocus = null;
            if (i != 0) {
                frm["EXPLANATIONVEH" + i].disabled = false;
                frm["EXPLANATIONVEH" + i].onfocus = null;
            }
            if (clickevent) {
                frm["NEWMAILINGADDRESS1" + i].value = '';
                frm["NEWMAILINGADDRESS2" + i].value = '';
                frm["NEWMAILINGCITY" + i].value = '';
                frm["NEWMAILINGSTATE" + i].value = '';
                frm["NEWMAILINGZIPCODE" + i].value = '';
            }
        }
    }

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

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

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.

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.

I'm curious to read the criticism on code.

Reply on news.ycombinator.com