• (disco)

    That sad thing is that this code:

    ERROR_REC Error_A_List[] = {
  {"AA01", "Error AA01 Description"},
  {"AA02", "Error AA02 Description"},
  /* about 100 more! */
};
ERROR_REC Error_B_List[] = 
{
  {"BA01", "Error BA01 Description"},
 {"BA02", "Error BA02 Description"},
/* more of course */
}
/* etc for all the letters! - 26 total 2D arrays*/

    Could be fixed by just using std::map:

    std::map<std::string, std::string> Error_List = {
  {"AA01", "Error AA01 Description"},
  {"AA02", "Error AA02 Description"},
  /* about 100 more! */
  {"BA01", "Error BA01 Description"},
  {"BA02", "Error BA02 Description"},
  /* more of course */
};

    https://ideone.com/7RZoWI

    This sort of thing is why I believe std::vector and std::map (and other standard containers) should be taught before ever even mentioning plain arrays.

  • (disco) in reply to LB_
    LB_:
    `std::map`

    I see nothing in the original code to suggest this is C++; it looks like vanilla C to me. It is certainly possible to implement a hashmap in plain C, and it's something a lower-division student learns (even as an EE student, Data Structures was a required CS class); it's just not neatly prepackaged for you.

  • (disco) in reply to HardwareGeek
    HardwareGeek:
    I see nothing in the original code to suggest this is C++

    Their C++ application tended to throw out a lot of error codes.

    it is literally the second sentence.

  • (disco) in reply to LB_

    :headdesk: Indeed it is. Mea culpa. I didn't go back and reread the narrative part of the article, I just looked at the code. And now that I look at it yet again, I see the bool data type. Absolutely nothing else looks C++-ish, though; char *, strcmp, strcpy — C++ only by virtue of its backward compatibility with C.

  • (disco)

    A bonus point in WTF for allocating dynamic memory just to release it afterward. In case a branch of the second switch is missed, the search will be performed with something that is not nullptr, but pointing anywhere (it might segfault, or in multithreaded environment it might have been reallocated by the time the pointer is used.)

    Lovely.

  • (disco) in reply to LB_

    I don't know. If the rate of errors is high and the number of messages is large std::map's operator[] and find are advertised as O(log(n)) in time complexity. Maybe it's better to use std::hash which is O(1). If the error rate is not all that high and there are only a few 100s of messages then you have to wonder how worthwhile it is to optimize the search... could just use std::find on an std::vector<std::pair<std::string, std::string>> and pay the (O(n)) cost.

    <!-- Easy Reader version: who cares about the performance of a low probability edge case? -->

  • (disco)

    If you ever give me a language without a Map type, it will be the first thing I write in that language. I'm a stringly typed guy.

  • (disco) in reply to RFoxmich
    RFoxmich:
    I don't know. If the rate of errors is high and the number of messages is large std::map's operator[] and find are advertised as O(log(n)) in time complexity. Maybe it's better to use std::hash which is O(1).

    In this case n is fixed, so O(log(n)) = O(1). Please don't abuse asymptotic complexity.

    In practice, it might well be the case that the cost of hashing the key is more than the few lookups done by a dichotomic search.

  • (disco) in reply to HardwareGeek

    delete[] isn't in C, is it?

  • (disco) in reply to Jerome_Grimbert
    Jerome_Grimbert:
    Lovely.

    Yeah, we all love UB. Well, until the code has to be ported to a DS9000, at which point someone (or more likely a million someones) dies in a nuclear fireball.

    I guess it's the "well, I have to initialise this pointer to point to something" thing, except that he hasn't heard of either NULL or nullptr, so he grabs a pointer from the only place he knows, the heap.

    I've seen enough fallout from UB to refuse to sign off on a code review if I find any, from *bp++ = expression_with_star_bp; in C to printf("%s\n", non_pod_object_by_value); in C++ (both of which I've seen) and more. (The second example is UB if the object is not a POD, and maybe - don't remember - if it is a POD. Version 6 of the Sun Solaris compiler passed the address of the object, while version 8 passed it by value and barfed warnings about it. The chuckleheads who wrote that code had to backpedal and rewrite it to do something sane.)

  • (disco)

    a total of 62 branches worth of case statements

    :wtf:

  • (disco) in reply to Planar

    In this case n is fixed, so O(log(n)) = O(1). Please don't abuse asymptotic complexity.

    That's about the silliest thing I've read today.

  • (disco) in reply to NedFodder
    NedFodder:
    >a total of 62 branches worth of case statements

    :wtf:

    Yeah, I wondered about the version of arithmetic that gets 62 from two times 26.
  • (disco)

    The article says "...it couldn’t do anything about other than cough up an error and quit". If that error description lookup happens only before quitting, why optimize it? OK the code is ugly and hard to maintain, and spends say 10 ms instead of 1. How much developer's time it's worth to spend on 9ms improvement? Is it algo trading industry?

  • (disco)

    Talk about premature optimisation.

    If your code has a fatal error often enough that a linear search of an array containing a few thousand strings is a performance concern, then you have bigger problems than your search strategy.

    In the context of big-Oh O(n) or O(log n), then 2600 elements is not exactly a huge number and there's no guarantee an asymptotically-faster algorithm will be faster.

    A routine that takes n iterations, will be faster than on that takes 1000 log2(n), whenever log(n)/n > 1/1000 -- so about 15000 elements. With memory latencies and cache misses, those relative timings are by no means unreasonable with a data structure like a map that stores the data with a bunch of non-contiguous heap allocations.

  • (disco)

    Yeah, this is a heavy case of why bother to optimize. The only thing you reasonably want to optimize here is the amount of boilerplate code.

  • (disco)

    Done in Java because I'm not up on C++ and I don't want to learn it for a stupid joke.

       String res;
   if (wk_code.equals("AA01")) 
   {
       res = "Error AA01 Description.";
   }
   else if (wk_code.equals("AA02"))
   {
        res = "Error AA02 Description.";
   }
   // about a bazlillion lines omitted
   else
   {
        res = "Unknown error."'
   }

    ...runs away laughing maniacallyevilly.

  • (disco) in reply to HardwareGeek
    HardwareGeek:
    :headdesk: Indeed it is. *Mea culpa.* I didn't go back and reread the narrative part of the article, I just looked at the code. And now that I look at it yet again, I see the `bool` data type. Absolutely nothing else looks C++-ish, though; `char *`, `strcmp`, `strcpy` — C++ only by virtue of its backward compatibility with C.

    I love C. It's simple, straightforward, fast, and sometimes even more elegant than equivalent C++, for short and sweet algorithms. But even I know that bodging a bunch of C into the middle of a C++ app isn't going to do anything but engender hate and loathing. I wish more people knew when to stick with what they like, and when to branch out.

  • (disco)

    we have a first-year CS major’s solution to breaking out of a while loop

    Or a first-year CS teacher's solution. Some people apparently think break statements are as evil and non-kosher as gotos.

  • (disco)

    Am I the only one to see this... and think: Every time something gets changed means a recompile.

    Would't it be better to put the error codes in some kind of lightweight text file? Simple database? That way you can add codes and update descriptions without having to recompile?

    You could even use XML and make it enterprise-y.

  • (disco) in reply to emkael

    The sad thing is that in C++ goto isn't that evil because unlike Java there is no labeled break or continue, so you need it to replace that functionality. IIRC there are no plans to add labeled break or continue to C++.

  • (disco) in reply to LB_
    LB_:
    The sad thing is that in C++ goto isn't that evil because unlike Java there is no labeled break or continue there's an impoverished set of control-flow primitives by comparison with a language like Lisp, so you need it to replace that functionality.

    FTFY :trophy:

  • (disco)

    First, a WTF in the article:

    ERROR_REC Error_A_List[] = {
  {"AA01", "Error AA01 Description"},
  {"AA02", "Error AA02 Description"},
  /* about 100 more! */
};

    This isn't a two dimensional array. This is a one dimensional array of ERROR_REC objects. Which probably are some kind of (const char*, const char*) pairs.

    The next :wtf: is why would you split this into 26 different arrays? Ok, the mapping from name to description has to be written down somewhere (although maybe as suggested above it could be in a file instead of in code). But you can just have one big table and then do the search there.

    The biggest head-banger, though, is that horrible search function. This is something that should be a couple lines long (mostly depending on how you stored the descriptions). If all you had was an unsorted array, for example, you just do:

    #include <algorithm>
#include <cstring>

struct ERROR_REC { const char* code; const char* description; };

ERROR_REC error_descriptions[] = { /* all the descriptions, in any order */ }

const char* GetErrorDescription(char *wk_code) {
  auto errorsCount = sizeof(error_descriptions)/sizeof(ERROR_REC);
  auto itToRecord = std::find_if(error_descriptions, error_descriptions+errorsCount,
      [wk_code](const ERROR_REC& rec) { return strcmp(wk_code, rec.code) == 0; });
  if (itToRecord == error_descriptions+errorsCount) return "Unknown Error";
  else return itToRecord->description;
}
  • (disco)

    What is wrong with having an error message like "AA01: Error AA01 Description"? No need to have any hashmap when application is about to quit anyways! If there is an error which:

    it couldn’t do anything about other than cough up an error and quit.

    Maybe it is time to quit quickly! What if that error is because of low memory, and additional memory allocation will result in OOM, that will silently quit defeating the whole purpose of having an error message to begin with.

    WernerCD:
    Would't it be better to put the error codes in some kind of lightweight text file? Simple database? That way you can add codes and update descriptions without having to recompile?
    What? let the poor application die in peace quickly.
  • (disco) in reply to dse
    dse:
    What? let the poor application die in peace quickly.
    Having a file with error codes doesn't mean you have to load the file when you're dying. You could build the map on startup (or have a static array) and then you make no further allocations to find and report the error.

    Although the fact that the error code is a string itself seems weird to me, I would have expected a number.

  • (disco) in reply to Kian

    Good point, yes during initialization makes sense, that also could be used for i18n.

  • (disco) in reply to HardwareGeek

    bool is in C; 7.18 Boolean type and values <stdbool.h>. Of course, most places that use C continue to act like it's still the 80's.

    I also like how ret_desc isn't declared within the function, meaning that it's almost certainly namespace scoped with static lifetime, unless it's declared thread_local hahaha of course it isn't. Hope you didn't want to get the description of more than one error message, or report on errors from two threads at the same time!

  • (disco)

    The problem was that their customers weren’t particularly happy with messages like: Error: QB57, since it told them absolutely nothing.

    So, "Error QB57 Description" made them happy? One stupidity deserves another.

  • (disco) in reply to UchihaMadara

    Presumably the descriptions were either placeholders or anonymized prior to the story being uploaded.

  • (disco) in reply to Steve_The_Cynic
    Steve_The_Cynic:
    gets 62 from two times 26.
    Are there only 26 alphanumeric characters? Hmm...
    1. a
    2. b
    3. c
    4. d
    5. e
    6. f
    7. g
    8. h
    9. i
    10. j
    11. k
    12. l
    13. m
    14. n
    15. o
    16. p
    17. q
    18. r
    19. s
    20. t
    21. u
    22. v
    23. w
    24. x
    25. y
    26. z
    27. 0
    28. 1
    29. 2
    30. 3
    31. 4
    32. 5
    33. 6
    34. 7
    35. 8
    36. 0
    37. A
    38. B
    39. C
    40. D
    41. E
    42. F
    43. G
    44. H
    45. I
    46. J
    47. K
    48. L
    49. M
    50. N
    51. O
    52. P
    53. Q
    54. R
    55. S
    56. T
    57. U
    58. V
    59. W
    60. X
    61. Y
    62. Z

    Huh. I guess 2*26 really is 62.

    Filed under: Maybe you meant 2*26+10?

  • (disco) in reply to Tsaukpaetra
    Tsaukpaetra:
    Are there only 26 alphanumeric characters?

    In this case, yes:

      switch(wk_code[0]) {
    case 'A': _wk_arr_size = sizeof(Error_A_List)/sizeof(ERROR_REC);    break;
    case 'B':  _wk_arr_size = sizeof(Error_B_List)/sizeof(ERROR_REC);   break;
/* etc for C through Z - who doesn't love a 26-option case statement? */
  • (disco) in reply to Dreikin
    Dreikin:
    yes
    On retrospection, it does seem that a clerical error has occurred. I guess it was interpreted that there would be numeric cases, when in fact there weren't.

    Filed under: Code not defensive enough, those edge cases need writing, Stat!

  • (disco) in reply to Tsaukpaetra
    Tsaukpaetra:
    Filed under: [Code not defensive enough, those edge cases need writing, Stat!](#Better_yet,_Include_all_other_symbols_too!)

    Indeed! Clearly, this coworker needs to at least include the codes for all printable characters in the Unicode standard.

  • (disco) in reply to Dreikin 
    ERROR_REC Error_ಠ_List[] = {
  {"ಠ_ಠ", "Error ಠ_ಠ ¯\_(ツ)_/¯"},
};

switch(wk_code[0]) {
   case 'ಠ': _wk_arr_size = sizeof(Error_ಠ_List)/sizeof(ERROR_REC);    break;
}
  • (disco) in reply to NedFodder

    I strongly doubt that'll compile.

  • (disco) in reply to PleegWat
    PleegWat:
    I strongly doubt that'll compile.

    Not exactly equal, but conceptually similar:

    #include <iostream>
#include <map>
#include <string>

std::map<std::string, std::string> Error_A_List = {
  {"AA01", "Error AA01 Description"},
  {"AA02", "Error AA02 Description"},
  /* about 100 more! */
  {"BA01", "Error BA01 Description"},
  {"BA02", "Error BA02 Description"},
  /* more of course */
  {"ಠ_ಠ", "Error ಠ_ಠ ¯\\_(ツ)_/¯"},
};

int main()
{
	for(auto const &value : Error_A_List)
	{
		std::cout << value.first << ": " << value.second << std::endl;
	}
}

    Produces:

    AA01: Error AA01 Description
AA02: Error AA02 Description
BA01: Error BA01 Description
BA02: Error BA02 Description
ಠ_ಠ: Error ಠ_ಠ ¯\_(ツ)_/¯
  • (disco) in reply to Dreikin

    Well, yes. But that doesn't include what is almost certainly a multibyte character literal where the compiler is expecting a single byte.

  • (disco) in reply to PleegWat
    PleegWat:
    Well, yes. But that doesn't include what is almost certainly a multibyte character literal where the compiler is expecting a single byte.

    New version:

    #include <iostream>
#include <map>
#include <string>

std::map<std::string, std::string> Error_A_List = {
  {"AA01", "Error AA01 Description"},
  {"AA02", "Error AA02 Description"},
  /* about 100 more! */
  {"BA01", "Error BA01 Description"},
  {"BA02", "Error BA02 Description"},
  /* more of course */
  {"ಠ_ಠ", "Error ಠ_ಠ ¯\\_(ツ)_/¯"},
};

int main()
{
	switch('ಠ')
	{
		case 'ಠ': for(auto const &value : Error_A_List)
		          {
		          	std::cout << value.first << ": " << value.second << std::endl;
		          }
		          break;
	}
}

    EDIT: Fixed the misquote. And then corrected the "fixed" misquote.

  • (disco) in reply to vkovalchuk
    vkovalchuk:
    delete[] isn't in C, is it?

    No, it isn't, nor is new(). It's an even worse mish-mash of C and C++ than I noticed at first.

    Filed under: You made me look at that terrible code again. I hate you.

  • (disco) in reply to Steve_The_Cynic

    I love the gcc take on passing non pods to ...

    It compiles successfully but with a warning (and who takes notice of those - no one where I work for certain) that the code will crash at runtime.

    That's possibly even sillier than playing tower of hanoi on an unknown pragma

  • (disco) in reply to WernerCD

    Exactly what I thought. Would also make localization easier.

  • (disco) in reply to NedFodder
    UchihaMadara:
    So, "Error QB57 Description" made them happy? One stupidity deserves another.

    I'm pretty sure that was redaction for the article. Nevertheless, you have a point, because the user is probably going from "Error QB75" to something like "Error QB57 A participle is not permitted at this thingamajig point in the binary query tree."

    But, hey, they'd better be happy: it shows them error messages now.

    NedFodder:
    ``` ERROR_REC Error_ಠ_List[] = { {"ಠ_ಠ", "Error ಠ_ಠ ¯\_(ツ)_/¯"}, }; ```

    That could have solved the whole problem right there (output) :

    "An error has occurred: error code QB57 ಠ_ಠ. Cause of the problem is ¯\_(ツ)_/¯."

    There...be happy with that.

    @Dreikin with apologies; wrote it before I saw your response.

  • (disco) in reply to Tsaukpaetra
    Tsaukpaetra:
    Dreikin:
    yes
    On retrospection, it does seem that a clerical error has occurred. I guess it was interpreted that there would be numeric cases, when in fact there weren't.
    You mean that you buttumed stuff and now you look like a butt.
  • (disco) in reply to Steve_The_Cynic

    Okay, I buttumed stuff and now I look like a butt. So? :stuck_out_tongue:

  • (disco) in reply to Steve_The_Cynic
    Steve_The_Cynic:
    buttumed stuff

    False.

    http://dictionary.reference.com/browse/assume 1: to take for granted or without proof:

    This implies without prior reference. However, in this case, information was internalized, analyzed, and applied into an interpretation, therefore it cannot be an assumption, as claims could certainly be sustained.

    Filed under: Do I get my :badger: now? I try SO HARD but nobody seems to care!

  • (disco) in reply to Tsaukpaetra
    Tsaukpaetra:
    Steve_The_Cynic:
    buttumed stuff

    False.

    http://dictionary.reference.com/browse/assume 1: to take for granted or without proof:

    This implies without prior reference. However, in this case, information was internalized, analyzed, and applied into an interpretation, therefore it cannot be an assumption, as claims could certainly be sustained.

    Filed under: Do I get my :badger: now? I try SO HARD but nobody seems to care!

    I answered a point about the number of branches in the two switch statements, and you answered as if I was talking about the number of different alphanumeric characters. Sounds like you assumed something.

  • (disco) in reply to Steve_The_Cynic
    Steve_The_Cynic:
    Yeah, I wondered about the version of arithmetic that gets 62 from two times 26.

    Okay, let's do some exploring then.

    https://en.wikipedia.org/wiki/26_(number)

    Relevant references relating to the article: The number of letters in the basic Latin alphabet, if capital letters are not distinguished from lowercase letters. Note that said switch statements branch off based on said characters of the Latin alphabet, therefore we can infer from the context that we are talking about letters.

    Furthermore, it is logical to infer from the array definitions above said switch statement that it would be possible to have numeric values in the error code, hence the simple test count for alphabet and numeric characters, of which the stated sum was equal to.

    What portion of my assertions are not without proof? Did you (at any point) claim that you were in fact talking solely about the number of branches in the two switch statements? Did you ever specify that you were indeed NOT talking about interpretations of ways that the number 62 could have been derived from 2*26?

    Filed under: I think not! Also Filed under: However, you are free to assume any opinion of my thoughts and words. @Blakeyrat any thoughts?

  • (disco) in reply to CoyneTheDup

    I did think of that possibility, but I wouldn't take it for granted. The universally accepted definition of a customer is "a moron who pays you some money." I wouldn't be surprised at all if a customer asks to replace "Error: QB57" with "Error QB57 Description" because it is more ... uhm, descriptive.

    That apart, even if the actual error was redacted, I wouldn't give the submitter the moral high ground, until I can see what the actual message was. It could well be that the submitter thought "Error: can't insert that thing into this thing" would be a useful error message. Nobody should have the right to share other people's poor code as WTF, unless they can prove that they are not TRWTF themselves to begin with.

  • (disco)

    Yes, I was particularly taken with the ingenious use of new() immediately followed by delete[], as alluded to by @Jerome_Grimbert.

    Also, typo in Remy's easy reader version (cut and pasted 26 times, of course):

    ERROR_CODE[0}
  • (disco)

    A couple of words of wisdom are needed here:

    "bsearch" and "qsort"

    Look them up in a manual page nearest you. Oh, they are in the C standard library and should be available in the library.

Leave a comment on “A Well Mapped Error”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article