- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
Frist
Admin
This isn't even the "good" version of this type of code, where you start from the high order digits and work your way down. Instead of multiplying by 10 (with integers) before adding in each new digit character's value they are doing a lot of extra type casting to use pow(). They can only do that because they managed to find the function to reverse a string, but apparently not the function to turn a string to an integer...
Admin
I'm dying to find out what's in that _tcsrev function. However, regardless of what's in it, how can they parse left to right by incrementing the pointer, but the logic looks like it's parsing right to left? Why would you ever parse right to left?
Admin
_tcsrev probably reverses the string, and that could be how it parses right to left by going left to right.
Admin
It seems that this code does not work for numbers like "-123" but only for numbers like "123-" which I have seen rarely in some sales reports. Also they check for a single space in front of the number, but that is dead code, because the surrounding if-clause already skips such a case. Then they use the slow pow-function and double values inside the loop instead of just using integers and multiplying by 10: oldValue=oldValue * 10 + newParsedValue. Why do they check for a return code of -256, and why in the beginning? Why is the string "-" allowed and parsed as 0?
Admin
You're probably right. It's bizarre because what about other things that need to be parsed? Does he reverse the string again? Wouldn't be surprised if there's a memory leak in the reversing, too. Like the article said, he was a college student, so I don't feel annoyed by this at all.
Admin
If this was written by a student and has worked for fifteen years without the need to change it; I think they got a very good deal. Imagine if it was written by a HPC, it would have cost 100 times as much and be broken 100 times sooner...
Admin
Is there a villain here?
It's not the student coder. They produced code that has lasted fifteen years without needing to be touched. That's pretty impressive. The fact that they're still working as a developer is also a very good sign.
It's not the company, unless there's another part of the story that we're missing. They provided work to someone who could use it. They were able to provide business value to their customers, successfully enough that fifteen years later, they're still around and able to employ people. And not only that, apparently at least some of the people who were there fifteen years earlier are still around and able to tell the story. That doesn't sound like a villain either.
Like the story three weeks ago about the CFO in the server room, this one honestly sounds like the opposite of a WTF. It's not a worse than failure, it's a better than success.
Admin
Apparently, I'm the only one who noticed that this line of code, bad as it is, is in fact even worse. Good luck if you ever have to port it to a successor of the System/360, e.g. today's latest and mostest shiniest zSystem...
You know, where
'0' != 0x30, and in fact the digit characters are negative because you forgot to turn on-f-unsigned-char...EBCDIC is still a thing.
Admin
But is that code or EBCDIC more likely to die?
Admin
It should be noted that having a symbol named _tcsrev is already undefined behaviour, because leading underscore is reserved for implementors by the C standard.
Admin
I personally admire using both '0' and 0x30 in the same function, but calling the integer result "retCode" gets bonus points as well, as does continually calling pow() instead of simply keeping a "powerOfTen" integer and increasing it by a magnitude each iteration.
Admin
Implementors such as Microsoft? https://docs.microsoft.com/en-us/cpp/c-runtime-library/using-generic-text-mappings?view=msvc-160
Admin
EBCDIC will never die and I used to write parsers like that in COBOL including decimals, negatives, dollars, per cents, you name it. Not even a subprogram, simple Copy code.
Admin
I like how the code only gets run if *p is a minus sign or digit, then the first thing it does is check if *p is a blank and skips it. (But only ONE blank, tyvm!) Actually, I guess that depends on what the mystery meat _tcsrev function does.
So how many times is this copypasta shit inlined in the code anyhow? I guess they didn't teach subroutines in CS101, you had to take CS102 for that.
Admin
Agreed. This shouldn't be filed under CodeSOD, it should be filed under Remy's Soapbox.
Admin
It's a string-reversal function supplied by the implementor, who are probably Microsoft. That it is working with
TCHARis probably a bigger WTF since that was definitely well known to be a ghastly hack at least 15 years ago.Admin
Heh! I wrote the program that did the heavy lifting in a project once, that used all sorts of baroque and curlicued functions to cover all possible eventualities. Fifteen years later we were trying to persuade the customer that they really did need to upgrade, because Old Fortran was seriously out of fashion, and they really needed to get up with the times and subscribe to their new Inner Platform driven new system ... but no, the customer had been running this system trouble-free since the day it had been installed, and why should they want to fix what ain't broken?
Dunno what happened in the end, I left the company soon after that conversation and moved on to me even more ineffective somewhere else.
Admin
What does this do with 1-2?
And I can see a reason not to use the library function--this one skips over spaces in the number. If you're going to do that, why not also skip over commas?
Admin
Admin
If EBCDIC was ever going to die, it would have done it by now.
Admin
(1) It's the first thing I noticed (2) If it dies, it's going to die in an atomic explosion. Even the dimmest CTO is going to notice this one. (3) It's a trivial fix, and the fix is orthogonal to an actual fix, which is to get the damn thing to use library functions (4) After fifteen years, it seems unlikely that the original code needed to be ported to an EBCDIC system. (5) Even more orthogonally, _tcscpy (the clue, for C/C++ programmers, is in the underscore) is an MSVC library function. So, "no" problem there.
Really, I don't know what all the fuss is about. I need to refactor five year old code (which only occasionally burps and/or wets itself) a couple of times a week. Give me precocious yet ignorant genius code like this every time!
Admin
OK, _tcsrev. But we're not seeing the calling site, which presumably (though not reliably over 15 years) uses the same library. The _tcsxxx calls, generally, require a length parameter (which is obviously not important if you're just reversing the string). Wonder of wonders, they are also sort of Unicode-compliant (thus the "t" for "TCHAR".
So, unfortunate as this is, it gets a few plus marks from me. I've seen worse (much worse) from Microsoft Research bods.
Admin
The way that code was created reminds me of my own entry into professional programming around the end of the '80s : Summer job after first year of university, got a student job at IBM. They still had consulting hours open with a customer, so I got sent there after a 2 week introduction to VM/CMS on 43xx series.
My first job, scoped to be completed during summer break: write a PL/1 Program that fetches information from a DB2 database and displays it as a diagram on a 3274 (?) terminal. My desk was buried under manuals for the whole of summer, but at the end of summer there was a working application that was in use until the whole Host system was retired. Still I shudder to think about the quality of code I must have produced.
Admin
One more thing. I've been in the business for thirty years, give or take.
I'm almost certain that none of my code, written fifteen years ago or more, is still in production.
I see the code in the OP as a triumph, not a WTF.
Admin
I think some people are missing the WTF. They had a student working on some code with no apparent oversight or feedback. I would assume (guessing, to be fair) that there were no unit tests. They probably threw a bunch of sample files at it, saw that the data was parsed correctly, and that was good enough for them*. And, apparently, they were right, but that may be good luck rather than good management.
*To be fair, it may be that the files being parsed could be considered reliable enough that extreme edge cases would never happen. Or worst case scenario, the number doesn't get parsed correctly, but it can still be fixed up manually. This may not have been a particularly core function for them.
Admin
Did we run out of actual WTFs? No over-engineered systems or solutions? No algorithmic Rube Goldberg machines? No insanely stupid security holes introduced through Custom Proprietary Super-Secret Encryption or SQL "sanitization"?
Sure, there are a few algorithmic and stylistic issues with the code. C code, written by a fresh graduate. It's been solid for 15 years. I've seen much, much worse written by some programmers with 30+ years of programming experience. I fail to see the WTF.
Admin
In addition to the points noted above, about it being supplied as part of the Windows API by Microsoft, the implementor, there's also the small point that your description of the rule is wrong. The correct rules about which underscores are reserved are:
An initial underscore followed by a capital letter.
A double-underscore anywhere in the identifier.
Addendum 2021-06-15 02:20: Might not be the Windows API itself, but if not, it's close enough that it might as well be.
Admin
When human civilization collapses after the nuclear exchange, this code and EBCDIC will outlive the cockroaches.
Admin
No we're back to criticizing inexperienced students' code again? Code that ran in production for 15 years without causing some cataclysmic disaster? No WTF here.
Admin
the first google hit for _tcsrev takes you to the manual page for _strrev, _wcsrev, _mbsrev, _mbsrev_l, which does make a passing reference to _tcsrev. This is why I gave up C programming (on Windows at least, in 1990's unix it didn't used to be too bad)
Admin
Exactly. 99.9% of programmers have their work being discarded or rewritten within 3 years of deployment. This thing fulfilled business requirements for 15 years. Yes the company does not have good practices in case, but they meet their deadlines and customer demands, so power to them I guess!
Admin
Hmm, seems to me the "natural" way to parse an integer is to proceed left to right, multiply the current value by ten, then add in the next digit. Instead this code calls a function that apparently reverses the string so that he can easily parse right to left, and then continually raises 10 to the power of the column number to get the column value. That is, he went to extra effort to reverse the string, to enable himself to go to extra effort to resolve column values.
Also why test for the string beginning with a space and skipping it when you've already excluded that possibility in the surrounding IF.
Admin
As others have noted, "ran for 15 years" is pretty impressive. My general philosophy is that code that runs and produces correct results is better than code that meets some standard of pristine beauty but doesn't work.
Admin
Admin
If you're talking about atoi() or atof(), I can understand why the student might coded his own string-parser: those functions don't error if the string is non-numeric, they take a best-guess approach. Eg: atoi("abc123def") returns 123, but it "should" throw an error. If your inputs might be "dirty" and you value correctness, this is a problem.
(At least, this is true in my working environment. The relevant C++ libraries are certainly outdated, but I don't think they are non-standard.)