Implications of a WTF



  • Here is a JavaScript code snippet I ran across the other day. There are things wrong with this on many levels, besides the obvious WTF, I'd like to see what we all have to say about broader issues. In fact I will say that the biggest problems are not the stupid, obvious, up-front bugs. I love this example because it is such a seemingly simple, innoccuous piece of kodekrap as to not warrant more than a passing "harrumph" yet the implications are important.

     <code>

    function RemoveCommas( number )
    {
        var str;
        var num = 0;
        if ( number != null && num.value != "" )
        {  
            str = number.value;
            for ( var i = 0; i < 3; i++ )
                str = str.replace( "," , "" );
            num  = str * 1;
        }
        return num;
    }

    </code>

    The Obvious Bug

    This code won't work for numbers like 1,234,567,890,765 because the function will remove only up to three commas.

    The Magic Number bug

    Programming Axiom: "Always use variables for constants." This better conveys their purpose and enhances maintenance. 

    The Superfluously Unnecessary Code bug

    Explicitly setting a variable to zero then immediately testing it for empty string? Priceless. 

    The "JavaScript Let's me be sloppy, so I will" bug

    A JS function signature does not include a return type. Note how this function returns a number sometimes or a string sometimes. We also write lots of C# code in our shop and of course we couldn't get away with this stupidity - "we know better"(?!), which makes it doubly dumb.

    The "I don't give a damn about client code that might use this" error

    See "... sloppy.. " error above.

    You may have heard about the technique / method of "coding (or code) contract" - if you haven't, you don't read enough. At our shop we (and almost all shops) don't implement that formally but I will say that unexpectedly passing back different data types is why we find it necessary to formally define such coding paradigms.

    The "Why is this function here anyway?" bug

    It is obvious (that's a given, please don't argue the point) this functionality should be in a library. One can imagine many individual web pages (and we have hundreds) needing unformatted numbers, particularly in preparation for doing arithmetic and comparisons. And one can imagine we already have such library code, and you'd imagine incorrectly.

    The "I don't want to improve my skills" bug

    The person who wrote this is an experienced,  skilled programmer and has the respect of his peers - including me. However this is virtually rote-code I've seen in our JS code in the past.

    Has it not occurred to the code-slinger that this function is less than optimal, even for it's limited purpose? Did not the coder's curiosity (a critical trait in the successful software developer) cause him to wonder about other ways to do this?

    The Perpetuating the Krap bug

    This function was going to code review in this state. And I can say from experience that it would not be questioned. As long as our monumentally stupid code analyzer does not give the code a score > ourITShop'sScoreLimit. 

    Here is the email reply I got when I brought up these issues:

    <quote>For the template he is using, it looks ok.</quote>

     

    DISCUSS!

    Code does become unmaintainable one blemish at a time. And it's hard to believe that code can deteriorate so badly that it literally cannot be fixed or added to. This was absolutely the case at a previous shop I was at so these "total maintenance" issues, as I will call them, are a pet peeve of mine. It's dishearting and embarrassing when a customer department head says to your face "we don't bring bugs to you because we know they will never be fixed."

    I'm really surprise that this was written by anyone in our shop. But over time I see that is is not uncommon enough. And a very recent new-hire has noticed this already. What thought process is the coder going through to write unprofessional code like this? (please don't say time crunch!!! BS squared!).

    I said this particular code example is very similar to older code - written about  3 years ago. What does this suggest about our testing? BTW I started using NUnit only recently and it was a self-initiative; no one else in our project uses it, or any other testing tool. Maybe I better start using JUnit too!
     

    Knowing a given language well doesn't mean one knows how to program well. These are distinct skills. I'll take the latter any day. For some reason we think they are the same and that simply learning more Java# makes one a better programmer. Everyone has lots of books on this or that language, but if you do not have a copy of Code Complete , for example, you prove my point.

    I'm surprised how many coders and coding shops simply do not get the big picture. We hire competent coders, have code reviews, etc. but even the sr staff let this kind of stuff simply slide by.

    An excellent coding shop requires leadership, not merely some been-here-a-long-time-decent-enough coder being promoted.  There is too much emphasis on management and almost none on leadership. I think knowledge workers need almost no management. The Lumbergs of the world impose TPS reports on us because they need something to do and because they certainly cannot lead. The typical management-inbreeding program is insidious.


     

     



  • You forgot a few:


    [b]The Internationalization Bug[/b]

    In many countries, a comma is used not as a separator for thousands, but as a decimal divider (the way English speakers generally use a "."). This code will fail in much of Europe and probably many other places as well. Even if you're going to run this code exclusively on a government server for citizens of a decimal-point-using country, there will be immigrants who become citizens. (Well, okay, not in Switzerland, but even in Switzerland there will be immigrants; they just won't be citizens.)

    [b]The Procrastination Error[/b]

    This is presumably used to validate a number which is being entered into a text field. Why wait until you're about to use the number to make sure it's in a format you can read? It makes more sense to use parseInt or parseFloat to correct the number as the user is filling in the form, as in:

    function correct_integer_input( the_input, the_error_display_id )
    {
      var x = new String( parseInt( the_input.value ) );
      if ( x == "NaN" )
      {
        x = "0";
      }
      if ( x != the_input.value )
      {
        ( document.getElementById( the_error_display_id ) ).innerHTML = "The value you entered has been altered because the format could not be understood. This field must contain an integer.";
        the_input.value = x;
      }
      else
      {
        ( document.getElementById( the_error_display_id ) ).innerHTML = "";
      }
    }

    ...

    <input name="some_field" type="text" value="0" onchange="correct_integer_input(this,'some_field_error')"><div id="some_field_error" style="color: red;"></div>

    I'm sure there are better ways of doing this (and there are definitely better error messages!) -- this was just to illustrate the point.



  • "procrastination error"  I like that term. 

    I believe that page forces input to be numeric; nonetheless that's a bad assumption to be making in this function.

     
    BTW, isNaN() is a great way to check for "numberness" in javascript.



  • Yes, but it's marginally better UI design to show the user what the computer will "see" instead of just saying "wait, the computer doesn't understand that", so it's better to actually parse the number and show the user what the value they typed will translate into.



  • I appreciate your need to vent, but --

    @radarbob said:

    The Superfluously Unnecessary Code bug

    Explicitly setting a variable to zero then immediately testing it for empty string? Priceless.

    The code you posted actually doesn't quite do that.

    The test relates to "num.value" with "num" being a Number object (created by "var num = 0") that, by default, doesn't have a "value" property, so "num.value" is guaranteed to be undefined in this context (but still a valid property lookup, even if it's pointless).

    I'd venture the guess that the author really wanted to write "number.value" (since the "number" parameter appears to actually relate to an HTML form element with a "value" property, not a scalar numeric value as suggested by the name) and was misled by his or her own confusing (lack of a) naming convention. "num" (a numeric value) in contrast to "number" (an HTML form element)? Sigh.

    @radarbob said:

    The "JavaScript Let's me be sloppy, so I will" bug

    A JS function signature does not include a return type. Note how this function returns a number sometimes or a string sometimes.

    I don't see that.

    The function you posted has exactly one "return" statement, and that statement always returns a number -- either the "0" value "num" was initialized with, the numeric result of the conversion performed by the function (the "num = str * 1" serves as a poor developer's typecast), or the pseudo-numeric "NaN" value if the input value does not happen to look like a valid number after stripping no more than three commas from it.


    That said, here's my shot at a slightly better and considerably shorter replacement (note that it expects a scalar value instead of an HTML form element reference like the original code did):


    [code]// Removes all commas from the stringified value of the argument
    // and returns its numeric value, if possible, or NaN otherwise.

    function RemoveCommas(number)
    {
      return Number(String(number).replace(/,/g, ""));
    }[/code]



  • @The Vicar said:

    but even in Switzerland there will be immigrants

    Not if the newly elected president has anything to do with it...



  • Hmm? Oh, that's [b]why[/b] immigrants in Switzerland aren't allowed to become citizens. Throughout the 20th century, the Swiss protected their economy by bringing in immigrants to do work during boom times, then forcing them out when things get tight in order to keep societal overheads low. (Notoriously during the oil shocks in the 1970s, for example.) The forcing is done by preventing naturalization; if things get bad, you just let their visas expire, then deport them. In the thesaurus of [i]realpolitik[/i], "ruthless" and "Swiss" are synonyms.



  • @radarbob said:

    Programming Axiom: "Always use variables for constants." This better conveys their purpose and enhances maintenance.
    wha? i will assume you mean "always use constants for constant values". using variables instead of constants doesn't sound like a good "programming axiom", much less doing it "always". and javascript even DOES have constants (even though i haven't really seen them used in any javascript in the wild - i guess most people are fooled by your "axiom" and use variables instead).



  • @gremlin said:

    @The Vicar said:
    but even in Switzerland there will be immigrants

    Not if the newly elected president has anything to do with it...

    I am Swiss, and I don't know what you are talking about. The elections happening 10 days ago was about forming a new legislative body. Therefore, the Nationalrat (which corresponds, I think, to your congress) and the Ständerat (should correspond to your Senate) were elected, not the president. Those two (the united federal assembly, consisting of both of the above councils) will elect the Bundesrat in about two months time (after establishing themselves). The Bundesrat, which is the national government, consisting of 7 ministers, will in turn choose their head (the Bundespräsident or the president of the federation). The latter will role will be passed around within the Bundesrat every year.

    Now who exactly is the newly elected president? If you would fill me in, please?   :-)

    [quote user="The Vicar"]Hmm? Oh, that's why immigrants in Switzerland aren't
    allowed to become citizens. Throughout the 20th century, the Swiss
    protected their economy by bringing in immigrants to do work during
    boom times, then forcing them out when things get tight in order to
    keep societal overheads low. (Notoriously during the oil shocks in the
    1970s, for example.) The forcing is done by preventing naturalization;
    if things get bad, you just let their visas expire, then deport them.
    In the thesaurus of realpolitik, "ruthless" and "Swiss" are synonyms.[/quote]

    There is quite some dispute going on within Switzerland about the issue. It isn't necessarily common agreement. The socialist party is getting attacked more and more for producing such a huge social welfare program that is feeding foreigners who never ever had a job there. OTOH, the fact that they were able to create such welfare programs (mainly in the big cities) is quite astonishing in and of itself.



  • I guess gremlin has been looking at too many UDC posters. Hmm, this is quite a topic twist.



  • @lanzz said:

    @radarbob said:
    Programming Axiom: "Always use variables for constants." This better conveys their purpose and enhances maintenance.
    wha? i will assume you mean "always use constants for constant values". using variables instead of constants doesn't sound like a good "programming axiom", much less doing it "always". and javascript even DOES have constants (even though i haven't really seen them used in any javascript in the wild - i guess most people are fooled by your "axiom" and use variables instead).

     

    Actually, the reason we use var CONSTANT instead of const is because "const" is not supported by all browsers. Took me a while to figure that bug out... 



  • The function looks like VB syntax thought, looking at the CamelCase instead of camelCase. But meh.

    @The Vicar said:

    This is presumably used to validate a number which is being entered into a text field. Why wait until you're about to use the number to make sure it's in a format you can read? It makes more sense to use parseInt or parseFloat to correct the number as the user is filling in the form

    Since this function exists with its current name, I'd assume it does take input from a form field, but if the argument number is, or can be, an int, then that's a WTF on a higher level.

    So if I stick to the assumption that the input is always a string, and stick to the function name as it is, then I would want to do no more, and no less, than:

    function stripCommas(numericInput) {
        return numericInput.replace(/,/g, "");
    }

    No NaN check, no null check, no nothing. This will fail on non-string input and perform no validation per se — but hey, that's not this function's business, as I stick to the function name.

    And then I'd kill the function entirely because why wrap one method in another? Just write a comment. :)

    //pre-format input to remove any commas
    numericInput = numericInput.replace(/,/g, "");


  • @BEdmonds said:

    Actually, the reason we use var CONSTANT instead of const is because "const" is not supported by all browsers. Took me a while to figure that bug out...

    Gotta love IE! 



  • @The Vicar said:

    [b]The Procrastination Error[/b]

    This is presumably used to validate a number which is being entered into a text field. Why wait until you're about to use the number to make sure it's in a format you can read? It makes more sense to use parseInt or parseFloat to correct the number as the user is filling in the form

    Validating as the user types is a poor UI. The whole point of having a textfield, navigable by cursor keys and mouse actions, is the ability to enter something non-sequentially. For instance, someone may be pasting from a web page or other application, and the clipboard may contain some alphabetic characters, which the user will erase after pasting it into the textfield. Always verify input when the user says he/she is done entering input, and not before.



  • IIRC, Switzerland uses ' for the separator, so even more possibility of breaking the javascript...



  • @VGR said:

    @The Vicar said:
    [b]The Procrastination Error[/b]

    This is presumably used to validate a number which is being entered into a text field. Why wait until you're about to use the number to make sure it's in a format you can read? It makes more sense to use parseInt or parseFloat to correct the number as the user is filling in the form

    Validating as the user types is a poor UI. The whole point of having a textfield, navigable by cursor keys and mouse actions, is the ability to enter something non-sequentially. For instance, someone may be pasting from a web page or other application, and the clipboard may contain some alphabetic characters, which the user will erase after pasting it into the textfield. Always verify input when the user says he/she is done entering input, and not before.


    You don't know your HTML event handlers. "onchange" only gets called when the user leaves the text field, allowing the user to paste in something and then clean it up.


  • You don't know your HTML event handlers. "onchange" only gets called when the user leaves the text field, allowing the user to paste in something and then clean it up.

    I hate those things. More often than not, they lock you into the field and prevent focus change until you give it a valid value, which you might not have or want to put in at the time. In worse cases, it even keeps you from clicking on links and navigating away from the page.

    Validate when the user says it's okay to validate. Not before.
     



  • The real WTF (the real, really real one -- other than the whole Swiss WTF)), is that javascripters still feel the need to code this crap.  Aren't there free masked edit controls like excentrics (http://www.eworldui.net) that keep you from having to write this utility code?

    I am honestly thinking javascripters love to code -- they code for the sake of coding, regardless of the value-add of the code that they so love to code.  And, man, is it easy to write hundreds of lines of code in javascript.  There is nothing like a design time syntax checker or strong typing to get in your way.  So they just code, and code, like they are getting paid by the line of code.  It is only now after 10 years of javascript hacks propagating like genital warts on the web that people are starting to build libraries like prototype. 

    But I know that there are a ton of javascripters who would say, screw it, I can write those functions myself!  Who needs those f-ing libraries and frameworks, and if we standardize on those, what will I be able to code? 

    'Cause I love to code, and code, and code, and place alerts everywhere, and sometimes forget to take them out of loops that are nested 10 deep, and then it is the client that sees my 'Got Here!' message, but so what, because accidents do happen, especially when you are coding, and you can code really fast in Javascript...yada, yada, yada.



  • @TunnelRat said:

    The real WTF (the real, really real one -- other than the whole Swiss WTF)), is that javascripters still feel the need to code this crap.  Aren't there free masked edit controls like excentrics (http://www.eworldui.net) that keep you from having to write this utility code?

    I am honestly thinking javascripters love to code -- they code for the sake of coding, regardless of the value-add of the code that they so love to code.  And, man, is it easy to write hundreds of lines of code in javascript.  There is nothing like a design time syntax checker or strong typing to get in your way.  So they just code, and code, like they are getting paid by the line of code.  It is only now after 10 years of javascript hacks propagating like genital warts on the web that people are starting to build libraries like prototype. 

    But I know that there are a ton of javascripters who would say, screw it, I can write those functions myself!  Who needs those f-ing libraries and frameworks, and if we standardize on those, what will I be able to code? 

    'Cause I love to code, and code, and code, and place alerts everywhere, and sometimes forget to take them out of loops that are nested 10 deep, and then it is the client that sees my 'Got Here!' message, but so what, because accidents do happen, especially when you are coding, and you can code really fast in Javascript...yada, yada, yada.

    shhhhhh 



  • @The Vicar said:

    @VGR said:
    @The Vicar said:
    [b]The Procrastination Error[/b]

    This is presumably used to validate a number which is being entered into a text field. Why wait until you're about to use the number to make sure it's in a format you can read? It makes more sense to use parseInt or parseFloat to correct the number as the user is filling in the form

    Validating as the user types is a poor UI. The whole point of having a textfield, navigable by cursor keys and mouse actions, is the ability to enter something non-sequentially. For instance, someone may be pasting from a web page or other application, and the clipboard may contain some alphabetic characters, which the user will erase after pasting it into the textfield. Always verify input when the user says he/she is done entering input, and not before.


    You don't know your HTML event handlers. "onchange" only gets called when the user leaves the text field, allowing the user to paste in something and then clean it up.

    You're right. I'm a novice Javascript guy. Though for some reason I was explicitly of the mind that "onchange" fired per-keystroke events. My mistake.


  • Discourse touched me in a no-no place

    @VGR said:

    @The Vicar said:
    @VGR said:
    @The Vicar said:
    [b]The Procrastination Error[/b]

    This is presumably used to validate a number which is being entered into a text field. Why wait until you're about to use the number to make sure it's in a format you can read? It makes more sense to use parseInt or parseFloat to correct the number as the user is filling in the form

    Validating as the user types is a poor UI. The whole point of having a textfield, navigable by cursor keys and mouse actions, is the ability to enter something non-sequentially. For instance, someone may be pasting from a web page or other application, and the clipboard may contain some alphabetic characters, which the user will erase after pasting it into the textfield. Always verify input when the user says he/she is done entering input, and not before.


    You don't know your HTML event handlers. "onchange" only gets called when the user leaves the text field, allowing the user to paste in something and then clean it up.

    You're right. I'm a novice Javascript guy. Though for some reason I was explicitly of the mind that "onchange" fired per-keystroke events. My mistake.

    Perhaps one of you is thinking of onfocus and onblur. I'm not at all sure which of you that could be however...


  • IE events

    Gecko events 

    FYI

    If you're going for as-you-type validation -- which can be done in an entirely unobtrusive way by colouring the field -- you can use the key events.



  • wha? i will assume you mean "always use constants for constant values". using variables instead of constants doesn't sound like a good "programming axiom", much less doing it "always". and javascript even DOES have constants (even though i haven't really seen them used in any javascript in the wild - i guess most people are fooled by your "axiom" and use variables instead).

    I guess I should just say "give names to your numbers".  As I am used to not using real, unfungable constants in JS my terminology gets a bit loose.

     

    JS has constants? Last I paid attention CONST (I think) was a reserved word, but not used. 



  • The real WTF (the real, really real one -- other than the whole Swiss WTF)), is that javascripters still feel the need to code this crap.  Aren't there free masked edit controls like excentrics (http://www.eworldui.net) that keep you from having to write this utility code?

    Hey tunnelrat? RU the I.T. Grunt blogger? Attitude sounds familiar. (if so, I really enjoy the blog)

    Your "insightful observations" are interesting. For the records I love to code, and I imagine so do you. However the fundamental error in this kind of thinking- tagging someone as a this-or-that-language programmer - is that, for someone like me (and you I expect) is that I am a professional software developer. And I don't mean i get paid to do it (I do), but it's my attitude and approach. That I happen to be using JavaScript is incidental. JavaScript, et.al. is a tool, it's not what I am. 

    I happen to be specializing in JavaScript (and C#) at the moment. At one time it was COBOL. My assertion above goes to the heart of the idea of a professional code-slinger. I.E. "If you code in COBOL you're nuts and must be a crappy coder." Ignorant bias notwithstanding; there is a way of building software that transends a language. Languages support construction paradigms to varying degrees and the professional programmer uses them to the best of the ability of the language. The question is, are you going to be a sloppy programmer because the language let's you or are you going to apply solid design and structure and robustness? That code snippet I posted is not JavaScript's fault.

    For example I heavily use JavaScript objects (no one else in our shop has bothered) - this has resulted in hundreds, and in one case thousands, of lines less code (while adding functionality!) than in earlier versions of certain programs I've worked on. In another job I "discovered" that we had a COBOL 85 compiler, no one in our shop had a clue about '85, but I learned: I rewrote unmaintainable, horrendously buggy programs that were now larger and more reliable. I could not have done that w/o the new features of COBOL 85. I bothered. No one else did.

     

    P.S. for the record we use third party JS libraries as well as roll our own.

    I too wondered sometimes why 3rd party libraries aren't more prevailent. Amatures, ignorance, and really really cheap-bastard IT shops. In a previous job our web tools were anything we could get free off the internet. One guy actually gave a presentation on building a web page with MS word. We even were using the free version of MS SQL. (If you are I.T. Grunt, you will really appreciate the fact that I'm talking about a state health agency!)  These, IMHO are mere symptoms of management's attitude (and profound ignorance). How can anyone develop professional coding skills in that kind of environment? The dumb just stay that way, and move around IT space to "spread the love."



  • Agreed.  It's just that, IMHO (if I was capable of a HO),  some languages seem more prone to hackishness, especially interpreted ones.  And if I can get away with it, I'll use a nice compiled strongly-typed language to get the job done.  Server-side C# over client-side JavaScript any day.  But even when I JS, I try to keep it clean and incorporate reuse and other good coding principles. 



  • I think what he was trying to say is that various langauges have a kind of stereotypical programmer using them.  Whether or not this has something to do with the language, the people that made the language, the community most involved with the language, or something else is a different matter.

     For instance, I would wager money that the average JavaScript code is much worse than the average Ruby code.  I would say that the average LISP code is probably better than the Ruby code.  I say this because JavaScript is the haven of pretty much anybody who wants to do something somewhat interesting with the user of a website, which is a massive group of people.  Furthermore, the lack of standardization has filled the code with hacks.  And it's harder to back away from the code far enough to start applying design patterns and architecture when you are constantly down in the nitty gritty making sure that your divs are properly lined up in 4 different browsers.

     

    Another thing is that a large percentage of JavaScript is presentation level code.  I don't know about you guys, but most places I look, the worst architecture is in the presentation layer of an application.
     



  • But if you use things like server-side controls like in ASP.NET and third party controls like ComponentArt, they handle all the cross-browser issues and even compress the JS.  It looks like a byte stream on the client, but you never have to deal with the messy details.  Yes, I know someone had to write those components, and there are pros-and-cons, but I am not a widget maker, I am applications developer.  I think.



  • @tster said:

    II would say that the average LISP code is probably better than the Ruby code.

    That one's easy. Lisp is difficult to use, so the only people who can use it are the good ones. Your average java or .net idiot just wouldn't be capable of it.

    Not the only reason why this kind of thing happens, but probably the main one where lisp is concerned.


Log in to reply