Comment On X-M-Hell

I'll bet I know what exactly what the author of this code (that Marvin Smit came across in a code review) was thinking. Here goes: [expand full text]
« PrevPage 1Next »

re: X-M-Hell

2004-09-13 13:00 • by Ben Hutchings
I love the way the string positions are typed as Double, just in case a target sub-string should start somewhere in the middle of a character in the document.

re: X-M-Hell

2004-09-13 13:05 • by Random Lurker
Since it comes from a code review, assume it's recent code and let's not having any defense saying VB didn't have the libraries at the time and get on to ridiculing this turd.

re: X-M-Hell

2004-11-15 17:36 • by Steven Padfield
Distribution difficulties are NOT the reason the author coded this method in VB. The author specifically explains the real reason: that he doesn't know of any other way to parse XML.

The fact that he would implement such an awful chunk of code based on his admitted inability to be aware of standard APIs is what makes this WTF.

re: X-M-Hell

2004-09-13 13:15 • by David Crowell
I like the UCase call on a string literal. That's nice.

re: X-M-Hell

2004-09-13 13:50 • by Marten Veldthuis
Ah well, at least it looks complex.

re: X-M-Hell

2004-09-13 13:50 • by anonymous me.
i actually worked at a place where my manager forbid us from using the MSXML parser in our VB6 application because he said it would take up too much space and cause more install headaches than we already had.... so we ended up writing all kinds of code like this.

in the end, our setup package was over 100Megs, and the project took weeks of debugging our xml parsing code.... nice timer saver. :P

re: X-M-Hell

2004-09-13 13:51 • by Derick Bailey
-- I like the UCase call on a string literal. That's nice.

you never know when ASCII or UNICODE stanards will change... a capital "A" might not be the same code as UCase("a") in a few years. :)

re: X-M-Hell

2004-09-13 13:54 • by Hassan Voyeau
Ben, I think the use of Double for the string position was used to deal with extremely long strings (and not substrings that start in between). However, the InStr function returns a Variant (Long) and the max length of a variable length String in VB is 2^31 (or 2147483648) which is max Long, so the use of Double was unwarranted.

re: X-M-Hell

2004-09-13 14:31 • by Ray S
Not only that, but for large enough values where using a double **might** be a good idea (in some alternate universe that is) you'll run into problems with floating-point inaccuracies and end up a few positions out from where you intended. Whoops!

re: X-M-Hell

2004-09-13 14:54 • by Hmm
Ray S: Not to mention it would probably cause InStr to crap itself too.

I also like the random German 'Zeug' in the function name, where everything else is in English. Apparently this coder has realized the ultimate truth that everything sounds more badass in German. Und keine Eier!

re: X-M-Hell

2004-09-13 14:58 • by Centaur
There was once a project I worked on that had floats for database table primary keys. “Now, please, fetch me record number ~3 million.” What a pity I wasn’t taking notes, it could be a worthy source of WTFness…

re: X-M-Hell

2004-09-13 15:07 • by Hassan Voyeau
RayS : I can't see how floating point inaccuries would cause problems if lets say the InStr function was modified to return a float and not a Long. Could you be more specific?

re: X-M-Hell

2004-09-13 15:22 • by Aarrgghh
Hassan: IEEE doubles are sixty-four bits. I don't recall how much of that is exponent, but whatever that value is, you can still use those 64 bits to represent numbers greater than 2**64. But you obviously can't express more than 2**64 discrete values when that's all the bits you've got. Out at the ends, you have to get "lossy". At some point, you're saying "uh, it's pretty close to 10**234" or whatever, instead of "it's exactly 8976".

If a double can reliably represent numbers greater than 2**32, it still might buy you something. That is, it would if XML were even halfway sane for files larger than 4.29 GB.

re: X-M-Hell

2004-09-13 15:38 • by Black
For me, i think the WTF would be the carefully crafted introduction to this ;). Congrats, you made my night (since im in europe and this gets published on evenings ;))

Hell, sign me up for a morning edition, so i can laugh the whole day ;)

re: X-M-Hell

2004-09-13 15:45 • by foxyshadis
Aarrgghh, do you work for a hard drive manufacturer? ^_~

The funniest part I see is that it obviously uses both namespaces and xlink, so there must be a powerful xml processor in the mix somewhere. Much more likely this is the work of someone who had never heard of xml and just saw it as another funny file format to write a custom parser for, same thing he'd done a dozen times before.

Note also the multiple uses of UCase(XML_Stream), along with the mid. Imagine the performance on a multi-meg data file there.

re: X-M-Hell

2004-09-13 16:04 • by Raymond Chen
Bwaaaa-hahaha. XML is case-sensitive!

re: X-M-Hell

2004-09-13 17:06 • by JBoy
Hey, at least he gave the method an appropriate name ... credit given where deserved.
;p

re: X-M-Hell

2004-09-13 17:30 • by Joshua
To my knowledge the MSXML parser doesn't support xlink to any degree. Perhaps this gentleman needed to retrieve data from an xlinked XML
document, and this was the only way to get at the url.

If you're just retrieving that single value, it doesn't make sense to incur the penalty of instantiating the parser.

This of course assumes he was limited to the MSXML parser. In addition, the number of un-necessary string operations gives one pause, but on the whole, it just looks like a quick and dirty function that does what is needed. Hell, the rest of the project might not even need an XML parser. It could just be a one off thing.

As much as I love laughing at some of the shite code that comes through here, I think this is a case that illustrates that software development isn't always as cut and dry as we'd like to believe.

re: X-M-Hell

2004-09-13 18:10 • by Thomas
Even by itself UCase("prefix:PublicationTarget") would be enough to justify a post :P

re: X-M-Hell

2004-09-13 18:19 • by Manni
When I go back to some of the first VB programs I designed, I bow my head in shame at the blatant use of Doubles just because I wanted my program to allow for the largest numbers possible without regard for memory usage or performance hits. There's also the logic flaw in that this guy was expecting an XML stream > 2 GB.

Then there's the hardcoded textual values to search for (which can NEVER change, right?) and the fact that the InStr function has that 4th parameter to do case-insensitive searches.

Those repeated UCase() calls are the sign of a true beginner. Everybody knows that if you are working with a 2 GB string as a parameter you should make a copy of it locally within that Function, and use UCase() on THAT.

re: X-M-Hell

2004-09-13 20:45 • by Joshua
As I said, the egregious use of ucase and the poor scoping of the string variable are most definitely questionable, and in fact, mockable. However, the implication was that using an XML parser would have been a more suitable course. I disagree.

I will, however, concede the point that this is a really poorly written routine from a pure code perspective, but the guy had the right idea at least.

re: X-M-Hell

2004-09-13 20:47 • by Joshua
s/concede/admit

re: X-M-Hell

2004-09-14 01:24 • by a fish
Hassan, Aarrgghh: IEEE doubles have (at least, iirc) 53 bits in the mantissa, plus one for the sign. So you're gaining 22 bits of precision.

re: X-M-Hell

2004-09-14 02:16 • by Hugo
Chr(34) may not work allways. XML may use single quotes as delimiter.

re: X-M-Hell

2004-09-14 02:23 • by Roger
<b> Derick Bailey
you never know when ASCII or UNICODE stanards will change... a capital "A" might not be the same code as UCase("a") in a few years. :) </b>

actually , in .net THAT IS TRUE!

if your on a turkish system and use localized string functions (default)

if you do "i".ToUpper() you dont get "I" , you get an "I" with a dot above...

so to get around this you have to specify invariant culture in the toupper functions..

//Roger

re: X-M-Hell

2004-09-14 03:24 • by Factory
Hassan:
For large enough floating point numbers, f+1=f, which can be a bit of a problem.. :)

re: X-M-Hell

2004-09-14 03:25 • by Cakkie
I must also credit him for using the variant datatype for the parameter of the xml stream. You never know what other people might pass to your function :)

re: X-M-Hell

2004-09-14 05:22 • by Marvin Smit
Oohh.. and to make the story complete:

The call into this function was done like

ID = GetPubTargetIDFromXMLZeug( mDOMInfo.xml )

;)

re: X-M-Hell

2004-09-14 05:55 • by MGC
* * * ************** ***************
* * * * *
* * * * *
* * * * *
* * * * **********
* * * * *
* * * * *
* * * * *
* * * * *
*************** * *

re: X-M-Hell

2004-09-14 06:23 • by KoFFiE
A collegue of mine also had to write an own minimal XML-parser, but that was due to portability reasons, try finding a portable AIX/OS/2 2.11/Win32/*nix XML library - good luck... Anyway, it was a "bit" more general purpose and more solid than this fabrication, and since this is written in VB, the author can't use the portability excuse now can he? :)

I could understand that you write such a thing if you don't wanna bloat the application with some XML library, and that's the only spot where you need XML (had such an app that had to fit completely in 200k staticly linked with rather hard memory constraints), but hey, you're talking about OCX/DLL/... hell with VB, would one dependency more really make such a difference? Also writing a bit more solid function wouldn't hurt, nor be that hard...

What I really love about this code is the Ucase usage, both on the whole XML-stream multiple times, as on the constant strings. When you look at the way the code is written, it can't be a very large XML-structure that is expected, so the performance hit in production would probably be minimal, but why-oh-why the Doubles?

@Marvin Smit: Am I correct when I suspect that the mDOMInfo object is some sort of XML-parsing thingy? (not too familiar with XML or VB) Then the author of this should be really really proud of himself, he just made the WTF of the week :D

re: X-M-Hell

2004-09-14 09:41 • by Jeff
The worst part is the XMLStream parameter declared as variant. If you're going to parse strings manually, at least make it clear that you are looking for a string as a parameter. The name -- XMLStream -- and the variant declaration give a very misleading indication of what this function expects and how it works. At the very least, declare it "XMLString as String" or something.

Variants .... ARRGHH !!

re: X-M-Hell

2004-09-14 10:39 • by Tim Smith
Doubles are a very bad idea here. It goes far beyond the number of bits in the mantissa. You also have to look at the operations being performed on them. Addition can be very problematic on values that differ greatly in their exponent. (i.e. 5e100 + 1e-20 = 5e100)

re: X-M-Hell

2004-09-14 12:23 • by Hassan Voyeau
OK. I have been enlightened -- http://www.sybase.com/detail?id=1012599

I don't know what I was thinking before :(

re: X-M-Hell

2004-09-14 12:30 • by Marvin Smit
@KoFFiE: Ehh, yeah. It's a MSXML DOM object (DOMDocument2) (not gonna fight over L1,L2,L3 compat) but XPath was supported in that time. (+- 2001)

@Jeff: 110% agree... Let's try to push in a IUnknown and see what happens ;)

re: X-M-Hell

2004-09-14 22:11 • by Daniel
Okay okay, I admit it, years ago I did have a go at writing my very own XML parser in VB, just for use on a particular project. In my defence I... no... wait, I have no defence. It was a dumb thing to do.

re: X-M-Hell

2004-09-15 16:03 • by Joe Bloggs
asdf

re: X-M-Hell

2004-09-15 16:09 • by Joe Bloggs
Ah this thing works... anyways

Sjees, what a pile of arrogant buggers you lot are. The number of times I have resorted to exactly that kind of solution.

XML is useful in the right places but zealots like you people stick it everywhere even when there's only a few parameters to be passed around.

Fuck that piece or crud called MSXML too. A royal pain in the arse that nobody should be put through.

Good on you "GetPubTargetIDFromXMLZeug" author. I too support the KISS principle. I suppose I should setup an organisation for non MS licking independent thinking programmers like us.

re: X-M-Hell

2004-09-15 21:50 • by Jeff S
It's not so much that this function exists; it's that it is so poorly written as noted several times in the comments (accepting a variant parameter, UCASE() on the constant, calling UCASE over and over on the entire input string instead of using INSTR's non-case sensitive option, using Double datatypes for string positions, etc).

I feel every good programmer should know how to write parsing functions; this guy, unfortunately, has a lot to learn. and not necessarily how to use MSXML, in my opinion.

re: X-M-Hell

2004-09-16 12:55 • by Marvin Smit
@Joe Bloggs:

Yeah, we know.. only MS has XML parsers.. that's a shame. To bad we can't get it to be a standard.

Maybe your "NMSLITP" can start up a standarization something?

Let us know.

re: X-M-Hell

2004-09-21 07:59 • by Craig Thrall
For a very portable (and fast) XML parser, see http://expat.sourceforge.net. It's very handy for situations like this and has bindings for just about everything.

re: X-M-Hell

2004-11-10 06:44 • by the fu_er
**********
]
**********
*************
]
*************
*****************************
]
*****************************
************
]
************
**********
]
**********

re: X-M-Hell

2004-11-10 06:51 • by JJ
|0|
| |
_| |___
_| | | | |
| | | | | |
| | | | | |
\ /
\ /
« PrevPage 1Next »

Add Comment