Comment On Spot The WTF: JavaScript Edition

omg!

reenter should not be hyphenated.

These are alphanumeric characters?



    _ @ - .

Brendan Kidwell:

These are alphanumeric characters?



    _ @ - .

If thats your biggest concern, I envy you

OK, I'm guessing that JavaScript already has some sort of isAlpha or isNumber function or functions that could have been used in the first place, right? (Of course, they probably don't count @,_,-, or . as alphanumeric.)

What is TRIM2 and how does it differ from plain old TRIM? And why doesn't its name indicate how it's different?

Wouldn't it have been easier to do the comparison as a range or set of ranges. Like if (n <= 'a') && (n <= 'z'), etc.? I mean, even if you have to account for EBCDIC, you can still use sets of ranges.

Why is the developer setting the variable to all uppercase after he's tested it against both upper and lower case letters? And why does he do it each time through the loop?

Um, does this function actually return anything? It doesn't look like it returns true or false, but I'm not a JavaScript programmer so maybe I'm missing it.

Also, I really like the alert message. "Only Alphanumeric are allowed." Alphanumeric what? And why did they capitalize "Alphanumeric?" Plus, it sure was courteous of them to erase the value the user entered so that they'll have to enter the entire string again.

And people wonder why, as a developer, I hate most software.

Ok...

1) Allowing non-alphanumeric characters in a routine called "IsAlphaNumeric"

2) Testing each character separately instead of the entire word with something like Regex

3) Testing each character with a huge, 66 part AND statement instead of something like comparing ASCII value ranges

4) displaying an alert for EACH INVALID CHARACTER IN THE STRING!!

5) Changing it to uppercase only AFTER the comparisons. Moving that before would save you, um, at least 26 comparisons

6) Is "&&" a short-curcuiting operator in JavaScript? If not, the letter 'a' would still compare 65 more times.

hehe. I'd be curious to see his TRIM2() function. Might have some equally tasty tidbits in there.... :)

Holy smokes.



1)No regular expression, let's just check every character.



2)We'll force the textbox's value to uppercase. Every time we check a character.



3)We'll alert the user EVERY TIME we encounter an unacceptable
character, even if there are 12 of them in the same text box. BREAK
statements are for suckers, and users need to be taught a lesson.



4)Explicitly return from the function when the control flow can't do anything BUT.

1 - hungarian
2 - hardcoded character values
3 - _@-. are not alphanumeric
4 - empty field will be treated as alphanumeric
5 - all-space field will be treated as alphanumeric
6 - name suggests will return boolean, instead opens an alert on failure.
7 - return where none needed
8 - name suggests _tests_ field value, instead it modifies it
9 - setting value to value uppercase inside loop
10 - unnecessary checks for lowercase characters (could move the uppercase to the start)
11 - opening window alert inside the loop

Hell. I give up. there's approximately 1 error per 0.5 characters in this. It's like shooting fish in a barrel.

Simon

John Bigboote:

Holy smokes.
3)We'll alert the user EVERY TIME we encounter an unacceptable character, even if there are 12 of them in the same text box. BREAK statements are for suckers, and users need to be taught a lesson.

I believe that once the field is set to blank, the loop will exit as i > cfield.value.length.

Anonymous:

What is TRIM2 and how does it differ from plain
old TRIM? And why doesn't its name indicate how it's different?

As a TRANSACT-SQL developer might say:



What is "TRIM"?  I only know of

LTRIM: trims all spaces from the beginning of the string

and

RTRIM: trims all spaces from the end of the string



TRIM2 must thus be a user function which trims both ends of the string (hence the 2)

the alert call is no big deal, it will happen only once, becuase just
after displaying the message, it helpfully clears the invalid data the
user typed in.

Actually, I just realized that by setting the text box to an empty
string, the loop will basically end because the loop counter will be
greater than the length of the string. Thus, no multiple warnings.



Ironic that one bad practice (not getting the textbox value into a variable) saved the developer from discovering another.

Thanks everybody for noticing my error while I was typin' and testin'.
Yes, I have actually popped this shit code into a web page.

Thereby keeping him from learning from his mistakes, and probably ending up typing the same thing again [:'(]

Then there's substringing instead of charAt() to add to the list.

Setting the field to blank.  "You typed one wrong letter so now I'll make you retype the whole thing."  What a pain in the ass.

I discovered the ebay auction in the comments of yesterday's WTF.

I found the concept interesting and was able to get immediate feedback from the seller about a split shipment, sending the ones to one place and the zeros to annother.

I blogged about it, but in the end, I use a product that has the ability to generate guids so I don't need to buy one at auction. http://weblogs.asp.net/andrewseven/archive/2005/06/23/EbayGuid.aspx

I guess regex'es are for suckers only!

The whole function could be replaced with simple check on /[^\w@\-\.]/

[;)]

In the VB world, we also have LTrim() and RTrim() for truncating extra spaces on the left and right side, respectively. To take care of both simultaneously, we have a function called Trim(). There are no such built-in functions for Javascript, so this guy clearly wrote his own. Except that by calling it TRIM2(), he thereby proves to us that it's *one better*. Maybe it washes your car as it trims off whitespace, who knows.

Here's a couple of WTFs (or, at least, design problems) nobody noticed yet:



1. The 'i' in the for loop. Because of the way JavaScript handles
variables, that 'i' is now defined as a global (to be local to the
function, it'd have to be defined with a 'var i' somewhere in the
function)

2. It's pretty easy to just set up a field to only accept AlphaNumerics
(even for values of alphanumerics that includes @, -, and .) in the
first place. Assign an event handler that returns false for keys
outside the range you want.



--AC

Noone seems to have covered this wtf: The whole logic is mixed in with the page implementation. This wtfery is probably copied and pasted for every form field that needs to be validated.

Stan Rogers:

Then there's substringing instead of charAt() to add to the list.

JavaScript has an indexer for strings, so you can just use:

n = cfield.value[i];

Let me offer an insight into how this was written:



1. The initial loop was written. It only dealt with uppercase.



I enter some random text that includes leading spaces.



Oops! It gives errors 25 times.



2. I'll fix it by trimming spaces.



Oops... that helped but still annoying.



3. Hmm it only handles uppercase.



Hmm I better convert the text to uppercase.



That works! (The test string had an initial cap...)



4. Maybe inserting a return statement would help.



Nope, I tried that. It didn't cause any errors, so I'll leave it there.



5. Maybe if I blank out the text field it will stop bugging me.



Perfect! Whoops it doesn't cope with lowercase letters at the beginning of a string...



6. Added 26 more comparisons.



That's how I should have done it in the first place.



Perfect!

Anonymous:

Noone seems to have covered this wtf: The whole logic is mixed in with the page implementation. This wtfery is probably copied and pasted for every form field that needs to be validated.

He passes the field in as a parameter

Free:

...

You might want to correct the spelling of 'speech' in your signature.  Unless you meant 's peach', then you'll probably need to explain what that means.

Anonymous:

Noone seems to have covered this wtf: The whole
logic is mixed in with the page implementation. This wtfery is probably
copied and pasted for every form field that needs to be
validated.

The form field object looks to be passed in as  'cfield' so  can actually be used for any form field.



Although what the 'c' stands for in the hungarianesque notation I don't know.

Anonymous:

Anonymous:

Noone seems to have covered this wtf: The whole
logic is mixed in with the page implementation. This wtfery is probably
copied and pasted for every form field that needs to be
validated.

The form field object looks to be passed in as  'cfield' so  can actually be used for any form field.



Although what the 'c' stands for in the hungarianesque notation I don't know.

Cocktostoen

Brendan Kidwell:

These are alphanumeric characters?



    _ @ - .

I'm guessing the author of the code was trying to validate e-mail addresses, so the real name of the method should have been isEMailAddress() (of course he probably then decided I think I'll use this method for all the other fields on the page too...so I don't have to rewrite all those tests)...

The answer to your question has a boatload of caveats to it.

_ @ - . are not letters or digits.

However, - and . can be part of a number (-1.23). So if you are looking at an individual character i.e. is this a letter or a digit the answer is definitely no. If the question is, is this character sequence a valid number? The answer is yes (unless decimals and/or negatives aren't allowed).

In Java Character.isDigit('-') returns false (as does '.'). however, Double.parseDouble("-1.23") returns a double of value -1.23.

strongarm:

Brendan Kidwell:

These are alphanumeric characters?

    _ @ - .

I'm guessing the author of the code was trying to validate e-mail addresses, so the real name of the method should have been isEMailAddress() (of course he probably then decided I think I'll use this method for all the other fields on the page too...so I don't have to rewrite all those tests)... The answer to your question has a boatload of caveats to it. _ @ - . are not letters or digits. However, - and . can be part of a number (-1.23). So if you are looking at an individual character i.e. is this a letter or a digit the answer is definitely no. If the question is, is this character sequence a valid number? The answer is yes (unless decimals and/or negatives aren't allowed). In Java Character.isDigit('-') returns false (as does '.'). however, Double.parseDouble("-1.23") returns a double of value -1.23.

What does this have to do with whether those chars are "alphanumeric characters"?

Granted it's the least of the problems, but I pity anyone who might have to i18nize that someday.

strongarm:

Brendan Kidwell:

These are alphanumeric characters?

    _ @ - .

I'm guessing the author of the code was trying to validate e-mail addresses, so the real name of the method should have been isEMailAddress()

[...]

If author was trying to validate e-mail address then [s]he is way off... [*-)]

I mean ....@This@Email@Add.ress.would.be_all.right...... (dots are included [:)])

but+this-valid-address@wouldnt.work [:(]

Anonymous:

Anonymous:

Noone seems to have covered this wtf: The whole
logic is mixed in with the page implementation. This wtfery is probably
copied and pasted for every form field that needs to be
validated.

The form field object looks to be passed in as  'cfield' so  can actually be used for any form field.

I'm sure that this didn't stop our heroic chicken slayer from copying/pasting this function into every page that needed it.

John Smallberries:

JavaScript has an indexer for strings, so you can just use:
n = cfield.value[i];

RTFM! [8-|]

In javascript, object String does not have indexer!

VBScript does.

Anonymous:

John Smallberries:

JavaScript has an indexer for strings, so you can just use:
n = cfield.value[i];

RTFM! [8-|]

In javascript, object String does not have indexer!

VBScript does.

RTFM if you want to, but I replaced one line with the other and it worked fine.

    //var n = cfield.value.substr(i,1);

    n = cfield.value[i];

I got a bonus wtf:



He used "cfield.value.length" inside the for loop condition. Now mind
you, it wouldn't work otherwise due to him setting the string to
''  possibly, but instead of checking the string length in the for
loop he should have broken out of the loop instead.

John Smallberries:

Anonymous:

John Smallberries:

JavaScript has an indexer for strings, so you can just use:
n = cfield.value[i];

RTFM! [8-|]

In javascript, object String does not have indexer!

VBScript does.

RTFM if you want to, but I replaced one line with the other and it worked fine.
    //var n = cfield.value.substr(i,1);
    n = cfield.value[i];

Yeah! n is always undefined now...  [:)]

Anonymous:

John Smallberries:

Anonymous:

John Smallberries:

JavaScript has an indexer for strings, so you can just use:
n = cfield.value[i];

RTFM! [8-|]

In javascript, object String does not have indexer!

VBScript does.

RTFM if you want to, but I replaced one line with the other and it worked fine.
    //var n = cfield.value.substr(i,1);
    n = cfield.value[i];

Yeah! n is always undefined now...  [:)]

Oh! Wait a second! You are running Mozila-based browser right?

This is Mozila feature and not a standard of javascript. IE doesn't like it... [;)]

 

Anonymous:

Oh! Wait a second! You are running Mozila-based browser right?

This is Mozila feature and not a standard of javascript. IE doesn't like it... [;)]

 

Indeed! What's IE?

(So mea culpa; empiricism takes another hit.)

:(

Anonymous:

Anonymous:

John Smallberries:

Anonymous:

John Smallberries:

JavaScript has an indexer for strings, so you can just use:
n = cfield.value[i];

RTFM! [8-|]

In javascript, object String does not have indexer!

VBScript does.

RTFM if you want to, but I replaced one line with the other and it worked fine.
    //var n = cfield.value.substr(i,1);
    n = cfield.value[i];

Yeah! n is always undefined now...  [:)]

Oh! Wait a second! You are running Mozila-based browser right?

This is Mozila feature and not a standard of javascript. IE doesn't like it... [;)]

 

Actually, if it works in Mozilla, then by definition it is standard JavaScript. It isn't ECMAScript or JScript, but it is
JavaScript. It isn't cross-browser, and in a read-only action it does
nothing that the cross-browser, ECMA-standard charAt() doesn't also do.

The biggest (blatant) WTFery is the alert WITHIN the loop.  Bad.

John Smallberries:

omg!

reenter should not be hyphenated.

Wrong! every grammar nazi knows that it should be reënter.

Charles Nadolski:

John Smallberries:

omg!

reenter should not be hyphenated.

Wrong! every grammar nazi knows that it should be reënter.

Every preschooler knows that one should begin sentences with "Capitalization". *



*[Placement on the period of that last sentence depends on the country you live in.]

Stan Rogers:

Anonymous:

Anonymous:

John Smallberries:

Anonymous:

John Smallberries:

JavaScript has an indexer for strings, so you can just use:
n = cfield.value[i];

RTFM! [8-|]

In javascript, object String does not have indexer!

VBScript does.

RTFM if you want to, but I replaced one line with the other and it worked fine.
    //var n = cfield.value.substr(i,1);
    n = cfield.value[i];

Yeah! n is always undefined now...  [:)]

Oh! Wait a second! You are running Mozila-based browser right?

This is Mozila feature and not a standard of javascript. IE doesn't like it... [;)]

 

Actually, if it works in Mozilla, then by definition it is standard JavaScript. It isn't ECMAScript or JScript, but it is JavaScript. It isn't cross-browser, and in a read-only action it does nothing that the cross-browser, ECMA-standard charAt() doesn't also do.

Is it documented in JavaScript?

Simon:

1 - hungarian

Please please please, let's not start this argument again.  We'll have another MSprogrammers vs Everybodyelse flame war.

Simon:

2 - hardcoded character values

And make this whole travesty worse by having something like LOWER_CASE_A used 50-odd times instead? WTF?

christoofar:

Charles Nadolski:

John Smallberries:

omg!

reenter should not be hyphenated.

Wrong! every grammar nazi knows that it should be reënter.

Every preschooler knows that one should begin sentences with "Capitalization". *



*[Placement on the period of that last sentence depends on the country you live in.]

Ah, but a capital letter doesn't necessarily have to follow an eclamation point ;)

no, VBScript doesn't have a string indexer, at least not something like



  c = s[n]



or



  c = s(n)



unless you know of some other syntax that I've not been able to find - you have to do this:



  c = mid(s,n,1)

I found 17 problems. Most were probably already mentioned, and some are consequences of others:





1) "bf"IsAlphaNumeric;


2) function takes a form control, when it should take a string;


3) function modifies the form control's value;

4) failure to use a regex;


5) substr instead of charAt;


6) flagrantly long "if" statement;


7) _, @, - and . are apparently alphanumeric;


8) shows a message box


9) ... with an incorrect capital letter


10) ... and a missing word;


11) modifies the control again


12) ... forcing the user to retype the entire thing;


13) focuses the control;


14) modifies the control again, this time to uppercase


15) ...on every iteration of the loop


16) ...*after* the if statement, which could have been simplified otherwise; and


17) does not return true or false!

Anonymous:

no, VBScript doesn't have a string indexer, at least not something like

  c = s[n]

or

  c = s(n)

unless you know of some other syntax that I've not been able to find - you have to do this:

  c = mid(s,n,1)

You're right!

My bad! [:$]

Anonymous:

Is it documented in JavaScript?

Yes. See the expanded definition of String in the JS 2.0 standard:



http://www.mozilla.org/js/language/js20/formal/notation.html#string



I believe it's also in the ECMAScript 4 Proposal, but that hasn't been published as an ECMA standard yet.

John Bigboote:

Anonymous:

Anonymous:

Noone seems to have covered this wtf: The whole
logic is mixed in with the page implementation. This wtfery is probably
copied and pasted for every form field that needs to be
validated.

The form field object looks to be passed in as  'cfield' so  can actually be used for any form field.



Although what the 'c' stands for in the hungarianesque notation I don't know.

Cocktostoen

This is where I knew my mouth was gonna be...

Correct.