Comment On Overruled by RemoveSpecialCharsExceptQuoteAmpersand...

"Paul adds: every string in the system is routed through these functions and accessed via an ObjectFormatter class with a single method (“Format”) which has a single argument (“Object obj”). The Format method is called via remoting, looks up the object’s type in an XML configuration file, and calls the configured formatter via reflection."

It's like the ultimate Enterprise Application!

PhillS:

"Paul adds: every string in the system is routed through these functions and accessed via an ObjectFormatter class with a single method (“Format”) which has a single argument (“Object obj”). The Format method is called via remoting, looks up the object’s type in an XML configuration file, and calls the configured formatter via reflection."

It's like the ultimate Enterprise Application!

it's only to justify the other Enterprise rule: if it's too slow, buy bigger hardware

So, what does the function do?
<g>

Can somebody tell that what are the special chars except quote, ampersand, apostrophe, open bracket, close bracket, comma, hyphen, full stop, comma and forward slash? (I suppose comma is mentioned twise in the list just to make sure.) My brain blew up when I was trying to find out the characters which the code removes. That code could be written with one regexp replace command.

I'm rather surprised by the characters the routine removes, but I suppose there is a good business reason for that. Or it could be totally random, but I frequently find it hard to tell those two apart anyway...

So what purpose does this serve?

That' sick.

Captcha: cognac - Ex-cat-ly what I need now...

Cope with IT:

That' sick.

Captcha: cognac - Ex-cat-ly what I need now...

... Ex-act-ly of course ...

Should have that Cognac now.

To establish a real efficient bottleneck, this has to be synchronized.

Despite the long function name this is not entirely what this function does, consider the else.

else if ((chrCode == 39)|| (chrCode == 96))

        {

            sb.Append(Convert.ToChar(39));

        }

the this is converting a 96 to a 39. The function name is wrong.

> That code could be written with one regexp replace command.

That would be even sillier.
Regular Expressions would seldom be easier to understand, and they are an order of magnitude slower.
But of course, the routine shown here is questionable.

"Paul adds: every string in the system is routed through these functions and accessed via an ObjectFormatter class with a single method (“Format”) which has a single argument (“Object obj”). The Format method is called via remoting, looks up the object’s type in an XML configuration file, and calls the configured formatter via reflection."

That's the only real WTF, no excuse for it. The method's implementation is not optimal but no WTF either. The method's spec sounds reasonable - remove input characters that could cause problems by allowing only those deemed safe (i.e. not the "ennumerating badness" antipattern). OK, the name is idiotic too. But the above, that's WTF gold.

Umm...numbers (codes 48-57) are special characters now?

I'm guessing that the XML has no dates, times, telephone numbers, postal codes, SSNs, indexes, hashes, ISBNs, UPCs, RFCs, cube assignments, irrational numbers....

Not Dorothy:

Despite the long function name this is not entirely what this function does, consider the else.

else if ((chrCode == 39)|| (chrCode == 96))

        {

            sb.Append(Convert.ToChar(39));

        }

the this is converting a 96 to a 39. The function name is wrong.

And you missed the (chrCode==39 in the "if" part so this will only fire if it is a 96 - converting a ` into a '.

The else if checks for char 39 and 96 but 39 will be caught by the if so it'll never get caught in the else if.

If you are forced to write such a retarded method you should at least do it properly.

CAPTCHA = B0RKX0rZZ!!11!!112!!!!1ONE!1!!!!!LOL!!11!!ROFLCOPTER!!111

titrat:

>Regular Expressions would seldom be easier to understand

s/[^-&'(),.\/0-9A-Za-z]//;

How's that hard to understand? ;-)

No, that is an or condition. || means or && means and.

You played yourself.

eerr... what about "RemoveInvalidCharacters"?

I personally use "ReplaceShit" in my libraries. Dunno, maybe a WTF. But i like that way.

The goggles, they do nothing!

Not Dorothy:

No, that is an or condition. || means or && means and.

You played yourself.

I read it that way, too, but I think what Zemm meant was that 39 is in the first part of the if, so it shouldn't ever be true in the else-if anyway.

At least they're using a StringBuilder

Does the 'else if' check for 39? Yes.
Does the 'else if' check for 96? Yes.

Therefore you can say the 'else if' checks for 39 and 96. There is a language outside of computers called english that some of us like to use.

The point still stands that if the chrCode is 39 it'll never make it to the 'else if'.

captch: COCK

Now you *know* it's time to leave!

Jackal von ÖRF:

Can somebody tell that what are the special chars except quote, ampersand, apostrophe, open bracket, close bracket, comma, hyphen, full stop, comma and forward slash? (I suppose comma is mentioned twise in the list just to make sure.) My brain blew up when I was trying to find out the characters which the code removes. That code could be written with one regexp replace command.

Hey, I think I found a bug: they don't check for character code 44 twice in the if statement. According to the function name this must be checked twice.

Very "stinky".

titrat:

> That code could be written with one regexp replace command.

That would be even sillier.
Regular Expressions would seldom be easier to understand, and they are an order of magnitude slower.

Regular expressions are not slower for this task than remote method invocation and reflection through XML.

GruffPelt:

Umm...numbers (codes 48-57) are special characters now?

I'm guessing that the XML has no dates, times, telephone numbers, postal codes, SSNs, indexes, hashes, ISBNs, UPCs, RFCs, cube assignments, irrational numbers....

Not to mention that characters 0 through 14 are not special characters...

titrat:

>Regular Expressions would seldom be easier to understand, and they are an order of magnitude slower.

You do need CS 101 refresh... This is bullshit, (compiled) regex execution is as efficient as it gets. Unless an idiot implement it, I guess.

What happens if (or rather, WHEN) new exception characters need to be added? Do they refactor all their code so their function name remains accurate?

Zygo:

titrat:

> That code could be written with one regexp replace command.

That would be even sillier.
Regular Expressions would seldom be easier to understand, and they are an order of magnitude slower.

Regular expressions are not slower for this task than remote method invocation and reflection through XML.

Both of which would still probably be needed in this design. He DID say that everything called this, so your choice is to duplicate that RegEx everywhere, add another component to all the apps/components that call this, or skinny down this code to use the RegEx.

Put another way, the choice of using a RegEx solves very little of this WTF.

Jackal von ÖRF:

That code could be written with one regexp replace command.

Some people, when confronted with a problem, think “I know, I’ll use regular expressions.” Now they have two problems.
—Jamie Zawinski, in comp.lang.emacs

lol I've wrote functions like this before

The only WTF is that Paul argued for an hour or two. It would have taken far less time to implement this version, a version that uses a regular expression, and some benchmarking test code.

It really doesn't matter which version the boss picks because the performance gain from the regular expression is going to be overwhelmed with the time it takes to do the remote call.

I know you're heart's not in it, but at least use switch for readibility.

"I was overruled by the longest serving developer and, as a result, forced to write the ‘...’ function. Seriously."

Perhaps he wrote it this way to get back at the "longest serving developer"?

And people still ask me what's wrong with escaping. And I still wonder why, if people have to escape text, they still regularly opt for having more than one escape character.

titrat:

>
Regular Expressions would seldom be easier to understand, and they are an order of magnitude slower.
But of course, the routine shown here is questionable.

'remove/replace chosen characters' regexps start being faster than corresponding series of ||'ed comparisons starting with a series of 8 or so comparisons.

The code is horrible for using magic numbers without any kind of comment. So tomorrow you decide you don't need the ampersand, so you... look up its charcode in the ascii table and seek the corresponding entry in the code?

If you absolutely, positively have to use 'magic numbers', COMMENT!
|| (chrCode > 64 && chrCode < 91) // A-Z
|| (chrCode == 34) // "
|| (chrCode == 38) // &
|| (chrCode == 39) // '
|| (chrCode == 40) // (
|| (chrCode == 41) // )
|| (chrCode == 45) // -

Also, don't use X > Y-1 && X < Y+1 when you mean X <= Y, X >= Y. I check chrCode > 64, 64='@'. Wtf is chrCode > '@' && chrCode < '[' ? Wouldn't chrCode >= 'A' && chrCode <= 'Z' be better?

The general idea of the procedure is a WTF, sure, but the real WTF is horrible execution of said retarded idea, resulting in code that is WTF Squared.

Here's another nice thing: "special" characters include everything that doesn't have a character value from 0 through 255. Which is almost, but not entirely, quite unlike ASCII. (Strings in .NET are Unicode, so it's not Latin-1 either.)

The "enterprisey" part is a WTF, but so is this. For the record, yes, a compiled regex *would* be faster, and I for one would find it vastly easier to read -- what you've got here can only be understood by virtue of the ridonkulously long function name.

One regex to remove undesired characters plus a call to String.Replace() to replace ` with ' (which is what the second if does) would be all it takes.

Some things are not comfortably done with regexen, but this is practically a textbook application.

brazzy:

"Paul adds: every string in the system is routed through these functions and accessed via an ObjectFormatter class with a single method (“Format”) which has a single argument (“Object obj”). The Format method is called via remoting, looks up the object’s type in an XML configuration file, and calls the configured formatter via reflection."

That's the only real WTF, no excuse for it. The method's implementation is not optimal but no WTF either. The method's spec sounds reasonable - remove input characters that could cause problems by allowing only those deemed safe (i.e. not the "ennumerating badness" antipattern). OK, the name is idiotic too. But the above, that's WTF gold.

But WTF do they do with chars between 15 and 31? They aren't even printable.

Holy crap! Adding bad code to an all ready rotting code base is a daily requirement! Add to that the requirement of doing everything one to three "billable" hours and you have my job in a nutshell.

Cloak:

But WTF do they do with chars between 15 and 31? They aren't even printable.

They remove them. WTF, did you not read the code?

etc:

Cloak:

But WTF do they do with chars between 15 and 31? They aren't even printable.

They remove them. WTF, did you not read the code?

Ahmmm, I'm sorry? Who did not read it?

public class SearchDictionary extends Dictionary {

    // field declarations go here ... 

 

    public SearchDictionary(SearchDictionary other) {

        // ... guess

    } 

 

    public Object get(Object keyObj) {

       if (!(keyObj instanceof String))

             return null;  

       String key = (String) keyObj;

       if (key.equals("ID")) {

            return searchKey.getSearchId();

       }

       if (key.equals("PARMS")) {

            return getSearchParms();

       }

       if (key.equals("HITCOUNTS")) {

              return this.hits;

       }

       if (key.equals("TOTALHITS")) {

             return "" + this.items.getCount();

       }

       if (key.equals("MESSAGE")) {

             return message;

       }

       if (key.equals("ITEMS")) {

             return items;

       }

       return null;

    }

    public Object put(Object key, Object value) {

        throw new UnsupportedOperationException();

    }

}

one suspects it might be several times faster not to mention clearer to have a global array that has the character mappings and do it all in one line, something like:

for i = 1 to In.Length do Out.Append( MapTable[ In[ i ] );

I'd use a character lookup table which would allow arbitrary character mappings (96 -> 39) and removal.

It'd be a lot quicker than this (less conditional jumps - just 2, one for the loop and one to check for the 'remove character' code) and easier to change rules.

You could even pass different lookup tables to the function for different rules then (but you'd have to change the function name, and call the lookup table 'RemoveSpecialCharsExcept...').

But, I suppose it wouldn't be Enterprisey enough.

(I'm still not entirely sure WHY you'd want to do this though...)

Drat, someone bet me to it by seconds...

Greg_H:

public Object get(Object keyObj) {

       if (!(keyObj instanceof String))

             return null;  

       String key = (String) keyObj;

       if (key.equals("ID")) {

            return searchKey.getSearchId();

       }

       /* ... */

       if (key.equals("ITEMS")) {

             return items;

       }

       return null;

    }

    public Object put(Object key, Object value) {

        throw new UnsupportedOperationException();

    }

I think we can all agree that this would be a lot more enterprisey if it was built on a ThrowableObject framework.

public void get(ThrowableObject key) {

   /* ... */

   if (key.tostring == "item") {

     throw item;

   }

   /* ... */

}

In C you can just use a char constant as an integer.

Instead of (chrCode==34) you can say (chrCode=='"'). Likewise for the other characters.

Not Dorothy:

Despite the long function name this is not entirely what this function does, consider the else.

else if ((chrCode == 39)|| (chrCode == 96))

        {

            sb.Append(Convert.ToChar(39));

        }

the this is converting a 96 to a 39. The function name is wrong.

I think this is the case where an ALT+0096 (tick mark) is placed instead of a single quote. (ALT+0039)

96 = `
39 = '

See the difference? Maybe that is part of the special characters they want to munge.

Zemm:

titrat:

>Regular Expressions would seldom be easier to understand

s/[^-&'(),.\/0-9A-Za-z]//;

How's that hard to understand? ;-)

y#A-Za-z0-9/.,()'&-##d;

TRWTF is that this site doesn't use a CSS width specification with scroll bars where necessary, rather than mangling the code so it doesn't compile.

Zemm:

titrat:

>Regular Expressions would seldom be easier to understand

s/[^-&'(),.\/0-9A-Za-z]//;

How's that hard to understand? ;-)

I guess you're being a bit sarcastic, but I find that pretty easy to read. If you know the purpose of a regex, deciphering it becomes much easier because there are really only a few sensible ways to write it.

Only thing I would change is to add the global flag:

s/[^-&'(),.\/0-9A-Za-z]//g