• Ejfsk (unregistered)

    Frist

  • JiffyPop (unregistered)

    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...

  • (nodebb)

    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?

  • a (unregistered) in reply to Mr. TA

    _tcsrev probably reverses the string, and that could be how it parses right to left by going left to right.

  • Code Refactorer (unregistered)

    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?

  • (nodebb) in reply to a

    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.

  • DQ (unregistered)

    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...

  • (nodebb)

    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.

  • (nodebb)

    retCode += (int)pow(10, (double)i) * ((int)*p - 0x30);

    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.

  • Thomas J. (unregistered) in reply to Steve_The_Cynic

    But is that code or EBCDIC more likely to die?

  • Thomas J. (unregistered)

    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.

  • dpm (unregistered)

    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.

  • I'm not a robot (unregistered) in reply to Thomas J.
    leading underscore is reserved for implementors by the C standard

    Implementors such as Microsoft? https://docs.microsoft.com/en-us/cpp/c-runtime-library/using-generic-text-mappings?view=msvc-160

  • Appalled (unregistered)

    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.

  • I dunno LOL ¯\(°_o)/¯ (unregistered)

    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.

  • Angela Anuszewski (google) in reply to jkshapiro

    Agreed. This shouldn't be filed under CodeSOD, it should be filed under Remy's Soapbox.

  • (nodebb) in reply to Thomas J.

    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.

    It's a string-reversal function supplied by the implementor, who are probably Microsoft. That it is working with TCHAR is probably a bigger WTF since that was definitely well known to be a ghastly hack at least 15 years ago.

  • Prime Mover (unregistered)

    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.

  • Loren Pechtel (unregistered)

    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?

  • Brian Boorman (unregistered) in reply to I dunno LOL ¯\(°_o)/¯
    I dunno LOL ¯\(°_o)/¯ wrote: Actually, I guess that depends on what the mystery meat _tcsrev function does.
    From Generic-Text Mappings in tchar.h:
    For example, the generic-text function _tcsrev, which is defined in tchar.h, maps to _mbsrev if you defined _MBCS in your program, or to _wcsrev if you defined _UNICODE. Otherwise, _tcsrev maps to strrev. Other data type mappings are provided in tchar.h for programming convenience, but _TCHAR is the most useful.
  • Fnord (unregistered) in reply to Thomas J.

    If EBCDIC was ever going to die, it would have done it by now.

  • Sole Purpose Of Visit (unregistered) in reply to Thomas J.

    (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!

  • Sole Purpose Of Visit (unregistered)

    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.

  • Been that Guy (unregistered)

    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.

    • Never touched a mainframe before
    • Never even heard of PL/1 programming language
    • No experience with SQL
    • No experience with 3274 Graphics capable terminals.

    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.

  • Sole Purpose Of Visit (unregistered)

    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.

  • Chris (unregistered)

    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.

  • Is it over? (unregistered)

    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.

  • (nodebb) in reply to Thomas J.

    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.

    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.

  • (nodebb) in reply to Thomas J.

    But is that code or EBCDIC more likely to die?

    When human civilization collapses after the nuclear exchange, this code and EBCDIC will outlive the cockroaches.

  • Donald Klopper (google)

    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.

  • a robot (unregistered) in reply to Mr. TA

    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)

  • AntichristBob (unregistered) in reply to jkshapiro

    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!

  • guest (unregistered)
    Comment held for moderation.
  • jay (unregistered) in reply to Steve_The_Cynic

    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.

  • jay (unregistered)

    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.

  • I'm not a robot (unregistered) in reply to jay
    Also why test for the string beginning with a space and skipping it when you've already excluded that possibility in the surrounding IF.
    Read it again.
  • Jon (unregistered) in reply to JiffyPop
    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...

    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.)

Leave a comment on “Experience is Integral”

Log In or post as a guest

Replying to comment #:

« Return to Article