• sjs (unregistered)

    Someone writing comments such as "Should I crash or not?" must use the absolute worst software in the world to think that crashing is acceptable, and is done on purpose.

    But that was by far the funniest part of the code. Many CS students simply cannot code so the rest doesn't surprise me at all. Hell, we had to write a Java pretty-printer in school (in Perl) and the teacher's solution made me go "WTF?!". It was tailored to her test cases. Those of us who actually did it properly (so it would work with programs on one line, for example) were utterly pissed off that we spent so much time on it. The students were the same, complaining that they didn't get Perl and didn't know how to write it. ... little did they know that the next assignment was the same app, but in C. snicker

  • Mikoangelo (unregistered)

    I'm surprised by the quality of the comments. He even explained that magic 1.

  • fist-poster (cs) in reply to Asgeir
    Asgeir:
    Skybert:
    cognac:
    vt_mruhlin:
    //-------------------------------------------------------------
    //Hex2Char: converts hex to a decimal. i.e. 0x0A becomes 'A'
    char UC_Crypt::Hex2Char(char nHex){
         switch(nHex){
              case '0': return 0;
              case '1': return 1;
    ...
              case 'F': return 15;
         }
         return 0;
    }
    
    ....
    If that switch is supposed to be c or c++, it features a nice bug. ;)
    Looks ok, works ok. What's the bug? (I don't count missing default or misleading method-name as bugs).
    If you pass any ascii value other than from the (0..9, a..f, A..F) set into it, it will return zero.

    And so exactly where does c or c++ come in? It does return a default and that is unfortunately chosen (I think it should crash, or at least it should consider that possibility), but it would work the same in pretty much any language supported a switch-like thing.

  • johnny (unregistered) in reply to Edss
    Edss:
    "Worse that failure" is what's worse than failure.

    It stopped being cool to dog on the name of this site abouuuut a month ago.

  • albert j (unregistered) in reply to David
    David:
    Software Engineering courses should be fair game. The course is usually a culmination of four to five years of study in computer science & programming. The next week that "student" could be writing production code for some company.

    not always true. at my school the first course in software engineering is typically taken the beginning of the 2nd year. most of my peers were terrible programmers after 5 years of school, imagine them after one.

    besides, most software engineers i know are reasonably competent but not outstanding programmers. i'm guessing this is because there is much more emphasis on the entire process rather than just writing efficient, clean code. of course this is just my personal experience, but i thought it was at least worth sharing. i thought it's pretty much accepted that just because you have a degree in computer science doesn't mean you're an out of this world, or even competent programmer. why should there be a difference with software engineering?

  • cavemanf16 (cs) in reply to Chambers
    Chambers:
    Horrible code is horrible code, and this is it.

    There is one simple solution for all of you complaining about recent submissions: Send in your own!

    Agreed. I am, to this day, quite guilty of being a real WTF coder when I code (which is rarely), but when I do code, I don't make mistakes THAT egregious. That was truly WTF coding - even if just by a student.

    I have my BS in Comp. Eng. Tech. (from DeVry, no less - blech!), and I can recognize that code as truly horrible. If I can recognize that as shitty code, than just about anyone studying software design, coding, engineering should be able to as well. The only excuse for that crap is being given a test the very first day of your first year in college to see if you have any understanding at all of how to program. Seriously. An array for a 1 char value???!!! That's just retarded, and you learn what arrays are for in some of the earliest programming classes you have.

    Sorry, but morons in college are still morons.

  • muhahaha (unregistered)

    I dunno about you folks, but here's how I do the hex string to integer conversions:

    int Hex2Dec(const std::string& HexString) { std::stringstream converter; converter << HexString; int value; converter >> std::hex >> value; return(value); }

  • vt_mruhlin (cs) in reply to cognac
    cognac:
    If that switch is supposed to be c or c++, it features a nice bug. ;)

    c++.... where's the bug?

  • KattMan (cs) in reply to David
    David:
    Software Engineering courses should be fair game. The course is usually a culmination of four to five years of study in computer science & programming. The next week that "student" could be writing production code for some company.

    Flaws like project management and inadequate testing and even overall design shouldn't be considered because they're taught IN the course. Anything regarding code (which they should have learned freshmen, sophomore, or junior year) is fair game. Put it up here and let us giggle a bit and remember all the silly code we wrote back then. Chances are (if they become or are currently a good programmer) they'll think the exact same thing, laugh it off, and go back to code.

    Would you consider that computer-genius nephew of-the-CEO programmer (who has no formal training whatsoever) a student? What about someone who has no knowledge of the language they’re using (such as C-Pound). They're still clearly learning. Why do they not get the no-student benefits? The fact that they hired them in the first place is one thing, but throwing up code written by someone like that would violate the "no student" philosophy.

    No, he is a student, Yes next week he could be out in the world, but he will be a junior now learning from those who have been in the field hopefully getting subjected to code reviews and refining his technique. Once he starts charging for his coding experience he is then fair game, at this time he gets nothing in return but ridicule.
    There is no telling if he thinks his code is good enough to get paid for, and that makes all the difference in the world.

  • vt_mruhlin (cs) in reply to fist-poster
    Asgeir:
    Skybert:
    cognac:
    vt_mruhlin:
    //-------------------------------------------------------------
    //Hex2Char: converts hex to a decimal. i.e. 0x0A becomes 'A'
    char UC_Crypt::Hex2Char(char nHex){
         switch(nHex){
              case '0': return 0;
              case '1': return 1;
    ...
              case 'F': return 15;
         }
         return 0;
    }
    
    ....
    If that switch is supposed to be c or c++, it features a nice bug. ;)
    Looks ok, works ok. What's the bug? (I don't count missing default or misleading method-name as bugs).
    If you pass any ascii value other than from the (0..9, a..f, A..F) set into it, it will return zero.

    Meh, not a bug so much as intentional design. It's well documented that the input is supposed to be hex, so I didn't see the need to add exception handling to code that....doesn't handle exceptions... Worst case scenario it comes back saying the guy's password is invalid. I suppose I could write an error log to help analyze any problems...

  • jisakujien (cs)

    The real WTF is that the function never ends because it lacks a closing brace, right?

  • Zero (unregistered) in reply to headshit
    headshit:
    "It wasn't pretty and it certainly wasn't efficient, but it worked. And that, folks, is worse that failure."

    Stop trying to shoehorn in the New! Improved! and Fantastic! name of the site. Seriously, what the fuck?

    You missed "Enterprisey!"
  • AnonCoder (unregistered) in reply to vt_mruhlin
    vt_mruhlin:
    Meh, not a bug so much as intentional design. It's well documented that the input is supposed to be hex, so I didn't see the need to add exception handling to code that....doesn't handle exceptions... Worst case scenario it comes back saying the guy's password is invalid. I suppose I could write an error log to help analyze any problems...

    Wrong way to think for a number of reasons IMO.

    Documents lie all the time. Rely on them at your peril. How many times do you actually check your documentation is correct after you've fixed a bug?

    I've done plenty of integration work where the customer swears blind that all their data confirms to a schema, dtd or some other agreed format and it never has.

    If your unit of code is given an invalid input and it gives back a response that could be valid it is fundamentally broken. Your design contract does not include lying!

    Basically validating the input costs you little and this good practise makes applications far more robust. It's simple to just throw an unchecked exception or whatever the C++ equivalent is, at least that way it will get logged somewhere even if that's on the error out. It's not simple and at least two orders of magnitude more expensive to debug something like this later.

    /end sermon :)

  • bremac (unregistered) in reply to android

    Rather unfortunately, that generates no warning for invalid input (just returns 0, which is undefined as to it's correctness), which is likely why this code came about - part of the spec. for the assignment may have included alerting the user about input errors. Ah well, it beats using regexes in an undefined order into several variables, which may or may not initialize everything, and crashing without warning if an option isn't recognized (certain open source project).

  • bremac (unregistered) in reply to bremac

    The above should have been about strtol, but apparently the quote part of my message vanished into the ether...

  • diaphanein (unregistered) in reply to muhahaha
    muhahaha:
    I dunno about you folks, but here's how I do the hex string to integer conversions:

    int Hex2Dec(const std::string& HexString) { std::stringstream converter; converter << HexString; int value; converter >> std::hex >> value; return(value); }

    While close, you off a bit - you don't check status of the stream to see if it failed. Take a look at boost::lexical_cast for a correct implementation using std streams.

  • iMalc (unregistered) in reply to vt_mruhlin
    vt_mruhlin:
    Asgeir:
    Skybert:
    cognac:
    vt_mruhlin:
    //-------------------------------------------------------------
    //Hex2Char: converts hex to a decimal. i.e. 0x0A becomes 'A'
    char UC_Crypt::Hex2Char(char nHex){
         switch(nHex){
              case '0': return 0;
              case '1': return 1;
    ...
              case 'F': return 15;
         }
         return 0;
    }
    
    ....
    If that switch is supposed to be c or c++, it features a nice bug. ;)
    Looks ok, works ok. What's the bug? (I don't count missing default or misleading method-name as bugs).
    If you pass any ascii value other than from the (0..9, a..f, A..F) set into it, it will return zero.

    Meh, not a bug so much as intentional design. It's well documented that the input is supposed to be hex, so I didn't see the need to add exception handling to code that....doesn't handle exceptions... Worst case scenario it comes back saying the guy's password is invalid. I suppose I could write an error log to help analyze any problems...

    Hey, he originally said that the bug was with the switch. But this return 0 comes after the switch, therefore I call foul.

  • Me (unregistered) in reply to johnny
    johnny:
    Edss:
    "Worse that failure" is what's worse than failure.

    It stopped being cool to dog on the name of this site abouuuut a month ago.

    Maybe not cool, but still valid.

  • Perry Pederson (unregistered)

    Heh-- that reminds me of an answer that a friend of mine put on a midterm college computer science test.

    We were studying for loops, and were asked to write a program (on paper) that would shift the characters around: The first line printed should print "A" through "Z", the next line "B" through "Z" with an "A" at the end, the next line "C" through "Z" with "AB" at the end, etc.

    My friend couldn't grok how to do this (two nested FOR loops was used in my solution) during the test, so he wrote:

    Print "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; Print "BCDEFGHIJKLMNOPQRSTUVWXYZA"; Print "CDEFGHIJKLMNOPQRSTUVWXYZAB"; Print "DEFGHIJKLMNOPQRSTUVWXYZABC";

    (etc.)

    He got half credit. The teacher put the clause "You must use loops" in subsequent tests.

  • Jon W (unregistered)

    No machine that code will ever run on will align the stack on less than two bytes, so his string is perfectly safe. Don't complain about working code! Go back to optimizing The Report, and give me some more face time in the office!

  • anon (unregistered) in reply to Perry Pederson

    Well, that's just about the type of code i'd come up with if halfway through the project I'd get really pissed about the lecture or my team mates and wanted to "show them" (most likely since my assignment was so simple that it's boring me to even think about it), which is what the article title implies.

    Somehow none of you believe that a student could write such code on purpose, It seems to me like a clear sign of "Dienst nach Vorschrift" (work-to-rule, although i'm not sure if that can have the exact same meaning: protest against some stupid rules and therefore doing exactly what's required but make it in any way possible different from what's expected)

  • chrismcb (cs)

    PLEASE reinstate the No Student Code rule. I am sure we can pick any class (whether it is SE, or a grad class) and get a months worth of WTF's. Sure, many of these people will go on to write their own professional WTF's one day, but until they leave the Ivory Towers, leave them be.

    I thought this site was supposed to concentrate on PRODUCTION WTF's. That is WTF's in shipping code, or in an internal process that is actually being used. Not in code someone wrote at 4 am for a 10am deadline that will never be used again.

    I used to interview students, most of them CS students. I started off asking them to write strcmp. I probably interview a couple hundred students in the few years I interviewed. ONE came close to an optimal answer, and less than 10% wrote something that even worked. About a 1/4 didn't even quite grasp the concept. And these were people who were graduating and getting a CS degree! One guy claimed he just wrote an XML parser, and he didn't understand the concept of comparing two strings...

  • Troy Mclure (unregistered) in reply to chrismcb

    I agree its so fucking weak to use student code. Doesn't matter in what class it is, or how many years they have been in school. Shit we all started somewhere. The stuff I used to turn in back then was horrible. Some was a combination of not understanding, but frankly the other 50% was that I'd rather get laid, drunk or do anything other than sit and write code. I'd still rather get laid, drunk or anything else but sit and write code - but paying the bills is nice too

    Th1s s1te suxors!

  • Ornedan (unregistered) in reply to chrismcb
    chrismcb:
    I thought this site was supposed to concentrate on PRODUCTION WTF's. That is WTF's in shipping code, or in an internal process that is actually being used. Not in code someone wrote at 4 am for a 10am deadline that will never be used again.
    Except SE project is supposed to result in software that is put to production use (usually by someone at the university, but sometimes by a company that commissioned the project). There really is no excuse for writing code this bad ever after having learnt your first real programming language, which for the student involved should have happened at least 1.5a before participating in the project.
  • brendan (unregistered)

    there's two ways you can do that (there is another one that uses c++ streams). there's

    int XMLFeed::Reader::HexToDec(std::string* hex) { char *isvalid unsigned long l = strtoul(hex->c_str(),&isvalid,16); if (*isvalid == '\0'){ // something happened here and needs resolution } return l; }

    or if you just want to waist time then:

    int XMLFeed::Reader::HexToDec(std::string* hex) { const char* chHex = hex->c_str(); int acc = 0; for(; *chHex; chHex++){ if ((*chHex >= '0') && (*chHex <= '9')) acc += acc - '0'; else if ((*chHex >= 'A') && (*chHex <= 'F')) acc += 10 + (acc - 'A') else{ // something happened here and needs resolution break; } acc = acc << 4; } return acc; }

  • Old Wolf (unregistered) in reply to vt_mruhlin
    vt_mruhlin:
    I hate it when stuff on WTF is essentially the same as stuff I've written.....
    The article code is way worse than yours. It buffer overflows every time -- yours only does half the time! ;)

    Rather than list mistakes I think it will be easiest to show an improved version and you can compare differences for yourself:

    char* UC_Crypt::Hex2Bin(const char* pHex, unsigned int nSize){
         unsigned int nOutSize = (nSize+(nSize%2))/2 + 1;
         char* pBin = (char*)malloc(nOutSize);
         memset(pBin, '\0', nOutSize);
         for(unsigned int i=0; i< nSize; i+=2){
              pBin[i/2] = Hex2Char(pHex[i])*16 + Hex2Char(pHex[i+1]);
         }
         return pBin;
    }
    
    Note #1: it is exceptionally poor design for C++ code (as this clearly is due to the '::' operator) to return a pointer to memory allocated with 'malloc'. So I am providing a C version. For a C++ version you should allocate and return the memory differently. If you want a version that can be used in both languages, write the C version and declare the function with extern "C" in the C++ header.

    Note #2: Your version read off the end of the input when the length was odd. My version 'solves' this by ignoring the last character of an odd-length string. A better solution would involve specifying what to do with odd-length strings (left-pad with zero? right-fill with zero? right-fill with F?)

    /* Caller must free return value */
    char *Hex2Bin(const char* pHex, size_t nSize)
    {
      char *pBin = calloc( (nSize + 1) / 2 + 1 );
      if ( pBin )
        for( size_t i = 1; i < nSize; i+=2 )
          pBin[i/2] = Hex2Char(pHex[i-1])*16 + Hex2Char(pHex[i]);
         
      return pBin;
    }
    
  • Old Wolf (unregistered) in reply to Gazzonyx
    Gazzonyx:
    You can't open a file in C++ without converting a string to a char*. At least not in the standard libraries, and not that I'm aware of; if someone else knows differently, please let me know! It's been bugging me for quite some time, now!
    That's correct; I believe the reason is so that when you include <fstream> and so on, you don't have to drag in all of the <string> header stuff. (Could be wrong though!)
  • Old Wolf (unregistered) in reply to brendan
    brendan:
    int XMLFeed::Reader::HexToDec(std::string* hex) {
       char *isvalid
       unsigned long l = strtoul(hex->c_str(),&isvalid,16);
       if (*isvalid == '\0'){
           // something happened here and needs resolution
       }
       return l;
    }
    
    I suppose you didn't ever compile this. 'isvalid' should be declared as such:
      char const *isvalid;
    Also, if (*isvalid == 0) afterwards then it is likely that the conversion succeeded. An error occurred iff (l == 0 && isvalid == the start of the string), or if l == ULONG_MAX and errno == ERANGE.
          else if ((*chHex >= 'A') && (*chHex <= 'F'))
             acc += 10 + (acc - 'A')
    
    Not good on non-ASCII machines. I think the switch..case is the most elegant solution; it will also be easiest for compilers to optimise.
  • Old Wolf (unregistered)

    Redux of WTFs (M = meta):

    #M1: Ragging on student code. #M2: Writing this function, when XML accepts the un-converted input. #M3: Writing C-style code in a C++ function. #M4: Not using standard library functions that already do this. #M5: Function fails to accept lower-case hex.

    #1: Pointer to std::string is a poor choice for function parameter. It should either be const reference to string, or pointer to const char.

    #2: Declaring a local array of 1 element. (Almost always a WTF).

    #3:

    strcpy (strDigit, (hex->substr(i,1)).c_str()); 
    should be
    strDigit[0] = (*hex)[i];
    . The posted version causes a buffer overflow. Note the ugly syntax required due to the poor choice of function parameter type.

    #4: Doing a string comparison instead of a single character comparison.

    #5: Behaviour on error condition leaves a bit to be desired. (At least he commented out the exit(1) !!)

    #6: Causes undefined behaviour if pow(16, power) > MAX_INT.

    #7: Using floating-point power function for integer exponentiation. The value could be wrong due to rounding errors (although I don't think this would happen on IEEE floating point).

    #8: Using 'int' for the sum.

    #9: (minor) 'power' could have been used as the loop counter, obviating the need for intHexLength;

  • Derrick Pallas (cs) in reply to Old Wolf
    Old Wolf:
    Redux of WTFs (M = meta):

    #M1: Ragging on student code.

    I'd just like to say I'm sorry: I guess I didn't realize there was an issue with using student code, which is what the above is. Mea culpa.

    (In other news, please stop sending me student code.)

  • com2kid (cs) in reply to KattMan

    I AM a CS student, and I knew better than to do that kind of crap since after about day 3 of my first freshman course.

    Ignoring built in functions that do the same thing (and I am surprised that the student didn't use a built in functions, we are /great/ at that! :-D ), not knowing the difference between a char and a string is.... ick. Beyond stupid. I was given so many freshman assignments that consisted of character manipulations, and having to understand the base fact that at heart a character can be interpreted as an integer, that I have no idea how any student in any CS program anywhere manages to slip through without that understanding.

  • Hmmmm (unregistered)
    strcpy (strDigit, (hex->substr(i,1)).c_str());    //1 = we are examining one digit at a time

    The return value of hex->substr(i,1) is a temporary std::string. Unless my understanding of C++ temporary lifetimes is incorrect (which it may very well be), I am pretty sure that the temporary std::string is destroyed before the call to strcpy().

    Apparently he got "lucky" and the memory allocator didn't crap on the string when deallocating it, otherwise the code wouldn't have worked. (Or the buffer overflow might have been larger than it was.)

  • TechGnome (unregistered)
    Zylon:
    What the fuck. WHAT the fuck. WHAT THE fuck. WHAT THE FUCK. FUCK FUCK FUCK FUCKITY FUCK-FUCK.

    Sorry, had to get that nasty "worse than failure" taste out of my mouth.

    Good thing some of us checked this after we got home and not at work.... sheesh...

    Lastchance:
    //Should I crash or not?????

    I'm putting this in all my code, in random locations, from now on.

    sjs:
    Someone writing comments such as "Should I crash or not?" must use the absolute worst software in the world to think that crashing is acceptable, and is done on purpose.
    I've got comments like that.... I've got SQL that's peppered with " -- Something the frack went wrong here..." doesn't necessarily throw an error or cause a fatality, but it was in a "things went so horridly wrong at this point, we don't know WTF to do here."
    sjs:
    But that was by far the funniest part of the code. Many CS students simply cannot code so the rest doesn't surprise me at all. Hell, we had to write a Java pretty-printer in school (in Perl) and the _teacher's_ solution made me go "WTF?!". It was tailored to her test cases. Those of us who actually did it properly (so it would work with programs on one line, for example) were utterly pissed off that we spent so much time on it. The students were the same, complaining that they didn't get Perl and didn't know how to write it. ... little did they know that the next assignment was the same app, but in C. *snicker*
    I remember during some of our tech training in the Air Force, we had to code an Ada app that would take a string and based on it, display an ASCII image of some sort. The litmus test was the entry string 1234567890, which was supposed to out put a Christmas tree. One guy couldn't get it to work right, so he hard-coded the output. The instructor came by and entered the "123457890" and got the correct output. So he entered "1357"... and got the same output. Needless to say the student was told to try again. fortunately we weren't in charge of any aircraft.

    -tg

    CAPTCHA Error: Word too long: diminutive

  • brendan (unregistered) in reply to Old Wolf

    no I didn't even compile this because I could not be bothered and I thought you would understand what it was ment to do. but anyway just to make you happy I'll fix it for you.

    int XMLFeed::Reader::HexToDec(std::string* hex) {
       char *isvalid
       unsigned long l = strtoul(hex->c_str(),&isvalid,16);
       if ((errno == EINVAL) || (errno == ERANGE)){
          // something happened here and needs resolution
          // also it is an error if the string is empty as this 
          // would mean that a required field is left blank.
       }
       return l;
    }
    Old Wolf:
    brendan:
    int XMLFeed::Reader::HexToDec(std::string* hex) {
       char  *isvalid;
       unsigned long l = strtoul(hex->c_str(),&isvalid,16);
       if (*isvalid == '\0'){
           // something happened here and needs resolution
       }
       return l;
    }
    
    I suppose you didn't ever compile this. 'isvalid' should be declared as such:
      char const *isvalid;

    and I suppose you didn't compile it either. that produces the following error:

    error: invalid conversion from 'const char**' to 'char **'
    Old Wolf:
          else if ((*chHex >= 'A') && (*chHex <= 'F'))
             acc += 10 + (acc - 'A')
    
    Not good on non-ASCII machines. I think the switch..case is the most elegant solution; it will also be easiest for compilers to optimise.

    Can I ask which char encodeing this won't work for? My code is still valid when used with EBCDIC, ASCII and uniCode. Also most compilers are that smart to detect such optimising.

    Also to be pedantic, you failed to find 2 errors in my code. First is that it only excepts upper case chars. secondly. the result would also be wrong as it is shifted to the right by 4 bits will cause the function to produece invalid results.

    int XMLFeed::Reader::HexToDec(std::string* hex) {
       const char* chHex = hex->c_str();
       char tmp;
       int acc = 0;
       for(; *chHex; chHex++){
          tmp = toupper(*chHex);
          acc = acc << 4;
          if ((tmp >= '0') && (tmp <= '9'))
             acc += acc - '0';
          else if ((tmp >= 'A') && (tmp <= 'F'))
             acc += 10 + (acc - 'A')
          else{
             // something happened here and needs resolution
             break;
          }
       }
       return acc;
    } 
    
  • ActionMan (cs) in reply to Joe Blow
    Joe Blow:
    Do we know which corporate entity that Alex sold the site out to forced the lame-ass name change?
    Grandma?
  • Old Wolf (unregistered) in reply to Hmmmm
    Hmmmm:
    strcpy (strDigit, (hex->substr(i,1)).c_str());    //1 = we are examining one digit at a time

    The return value of hex->substr(i,1) is a temporary std::string. Unless my understanding of C++ temporary lifetimes is incorrect (which it may very well be), I am pretty sure that the temporary std::string is destroyed before the call to strcpy().

    In fact it persists until the end of the statement it's created in, so the code is OK. (If the creating statement is a loop condition, it persists until that loop condition has been evaluated).

  • Pat (unregistered) in reply to Welbog
    Welbog:
    To you guys saying that soft eng students should be fair game: in my university, there's only one required course called "Software Engineering", and it's a second-year, first-term course. Not all universities are the same.

    Thank you for posting this!

    The software engineering course at my school had virtually no prerequesites and so you got all kinds of grade levels in there, from sophomore to seniors. I believe there was even a graduate-level section merged in with ours.

    The project was to write a conference reservation system based on PHP and mySQL. The few prerequesites for this class were classes taught in C++. The code I wrote was beyond horrible, namely because I didn't know much of any SQL and just did things the hard way by writing very simplistic queries and doing all the hard work in the PHP scripts. If I hadn't taken a perl class in the previous semester, I probably would have even struggled to learn PHP.

    None of my queries were parameterized and so they were all vunerible to SQL injection attacks. I simply didn't know any better at the time! The sad part is that I was far and away the best programmer in my group. By the end of the semester, it was at least functional and our project was finished. I'm not proud of that code, but at least I know what I did wrong. Now that I'm 2 days away from earning my CS degree, I feel confident that I could rewrite that system securely in a matter of days if I tried.

  • Old Wolf (unregistered) in reply to brendan
    brendan:
    just to make you happy I'll fix it for you.
    int XMLFeed::Reader::HexToDec(std::string* hex) {
       char *isvalid
       unsigned long l = strtoul(hex->c_str(),&isvalid,16);
       if ((errno == EINVAL) || (errno == ERANGE)){
          // something happened here and needs resolution
          // also it is an error if the string is empty as this 
          // would mean that a required field is left blank.
       }
       return l;
    }
    This is even worse. When strtoul succeeds, it does not set errno. (In fact the C standard explicitly specifies that no library function sets errno to 0). errno could still be ERANGE because it was set by some earlier function call.

    In fact errno is not even set at all if no digits were found (eg. the string "foo"). strtoul does not set EINVAL either. (That is, the C standard does not specify that it does -- however, any implementation of a function can set any non-zero value of errno that it likes).

    To detect conversion error you have to check the return value first. Only once you have determined the return value was ULONG_MAX would you check errno.

    You're right that the 'isvalid' should be pointer to pointer to non-const char. However you have repeated your error of not finishing the declaration with a semicolon.

    brendan:
    Old Wolf:
          else if ((*chHex >= 'A') && (*chHex <= 'F'))
             acc += 10 + (acc - 'A')
    
    Not good on non-ASCII machines.
    Can I ask which char encodeing this won't work for? My code is still valid when used with EBCDIC, ASCII and uniCode.
    I don't know of any such encoding, but it could exist in theory. Probably shouldn't have mentioned it.
  • Joseph Newton (unregistered) in reply to Mcoder
    Mcoder:
    ...

    But I fail to see a problem with the use of 'pow' (except for it being sligtly slow) and portability (there is no problem of architecture endianess here, there would be if he didn't used 'pow'), unless you want to port it to a different color coding.

    You don't? That's a problem. IMHO, programming is not primarily about the tools used, but about thought, and awareness of context, and the use of pow in this context betrays a complete lack of that quality.

    Look at the context. That 8-char string is broken down into two-char strings, right? Why the hell do you need the pow function to multiply the first digit of each pair by 16?!

    Once again, I want to thank Jerry Ross [Lane Community College, Eugene, OR] my instructor in CS 161, for his wisdom in running us through Leslie Ann Robertson's "Simple Program Design" for six weeks before introducing any language specific material.

    Think it through first--code later.

  • slamb (unregistered) in reply to Joseph Newton
    Joseph Newton:
    Look at the context. That 8-char string is broken down into two-char strings, right? Why the hell do you need the pow function to multiply the first digit of each pair by 16?!

    He wrote a HexToDec (which is a stupid name, by the way - "hexstring2num" would be better, or just "strtol" if you're into standard names), even though he's only using it for two-digit numbers. I don't think writing code slightly more general to be reused is a WTF.

    Of course, the power thing is what a normal person would accomplish more quickly and without requiring an FPU by "sum = 16*sum + value" or even "sum = sum<<4 + value" at the same spot of the loop, shifting over a hexadigit each iteration rather than building it in place. (It turns out that positive integer powers are just repeated multiplications. Who knew?)

  • Pesho (unregistered) in reply to TSK
    The problem with the code: little- and big-endian architecture

    Endianness has nothing to do with it. The problem is with the char[1], and he'll get away with it as long as the local frame is padded, which has been safe to assume for quite a while -- maybe since the 90-s, I'm not sure (not that the poor sod would know anyway ;)

  • Kevin Kofler (unregistered)

    It's no longer safe to assume with GCC 4.

  • austinjp (unregistered)

    The more I read this site, the more I worry about the quality of coders on here and the kind of programming practices this site encourages.

    THIS IS NOT A WTF!

    The programmer does not use the sscanf function, which may have been optimal (probably because they don't know about it -- which is perfectly reasonable). Instead they craft their own implementation of the obvious algorithm to do it. The implementation is very clearly documented and easy to follow (if a bit long). There is a subtle bug with the: char strDigit[1]; //store 1 digit in that the null terminator from strcpy will overwrite something, but that is a common misstake and not an example of some crazy code that makes me want to throw up my hands and throttle the guy.

    Anyway, I'm giving up on this site. Have fun!

  • Peter (unregistered)

    The Real Portability Bug bug here is in

    for (int i = 0; i < intHexLength; i++)

    declaring the local i within the for statement is not supported in various flavors of gcc

  • segfault (unregistered) in reply to dp.design

    while i was a horrible coder as a student, i would have never thought of doing it like this.

  • Anonymous (unregistered) in reply to chrismcb
    chrismcb:
    ...I used to interview students, most of them CS students. I started off asking them to write strcmp. I probably interview a couple hundred students in the few years I interviewed. ONE came close to an optimal answer, and less than 10% wrote something that even worked. About a 1/4 didn't even quite grasp the concept. And these were people who were graduating and getting a CS degree! One guy claimed he just wrote an XML parser, and he didn't understand the concept of comparing two strings...
    Perhaps the inadequate responses indicate that strcmp is not straightforward to pull off of the top of your head, given the context of a student in the midst of a job interview.

    Here's one solution...

    int strcmp( const char* a, const char* b ) {
    	while( *a == *b && *a != 0 ) {
    		a++;
    		b++;
    	}
    	return static_cast<unsigned char>(*a) -
                        static_cast<unsigned char>(*b);
    }

    What do you think? Flame/mock away...

  • AdT (unregistered)
    Comment held for moderation.
  • Marc (unregistered) in reply to Bob the builder
    Bob the builder:
    <color red="255" green="127" blue="0"/>

    Why write code when you can use XML the way it was intended?

    <color> <parts> <colorPart type="red" format="decimal" value="255" /> <colorPart type="green" format="decimal" value="127" /> <colorPart type="blue" format="decimal" value="0" /> </parts> <alpha>255</alpha> <name>Puke</name> <captcha>waffles</captcha> </color>
  • Old Wolf (unregistered) in reply to Peter
    Peter:
    The _Real Portability Bug_ bug here is in

    for (int i = 0; i < intHexLength; i++)

    declaring the local i within the for statement is not supported in various flavors of gcc

    Can you name a flavour of gcc that doesn't support it? gcc 2.95 did, as do all later versions. The code is correct according to ISO/IEC 9899:1999, which has been the C standard for 8 years now.

  • Old Wolf (unregistered) in reply to Anonymous
    Anonymous:
    int strcmp( const char* a, const char* b ) {
    	while( *a == *b && *a != 0 ) {
    		a++;
    		b++;
    	}
    	return static_cast<unsigned char>(*a) -
                        static_cast<unsigned char>(*b);
    }
    What do you think? Flame/mock away...
    You haven't declared "static_cast", and even if you had, how can it be less than "unsigned char" ?

Leave a comment on “That'll Show the Grader”

Log In or post as a guest

Replying to comment #:

« Return to Article