• LZ79LRU (unregistered)

    Klak

  • withad (unregistered)

    It doesn't excuse the function name or the if condition but I could imagine that "String" used to be some internally-developed string class that was later replaced with the typedef of std::string. I guess whoever did that got it all compiling but didn't clean up the now-redundant methods like this one.

  • Smithers (unregistered)

    It's a "toLower function.

    You call a function that converts all the lowercase letters to uppercase "toLower?" I impressed you came up with a name somehow worse than "intToString."

  • (nodebb)

    So I'm not a c++ dev so sorry if I'm incorrect here, but this looks like a toUpper function to me.

    97: a 65: A

    p_param[i] = j-32;

    p_param[i] = /a/97-32;

    Seems that it is taking 32 away from every character from 97 to 122, all the lower case characters.

    Result should be 97-32 == 65 /A/

  • (nodebb)

    Yes, indeed, citing it as really being "toLower" is a substantial fail on the part of the editor of the article.

  • Michael R (unregistered)

    It all makes sense when you take upper-case digits into account.

  • Michael R (unregistered) in reply to Michael R

    I mean when you take lower-case digits into account.

  • Smithers (unregistered)

    The name and the long list of == checks instead of a couple of </> are just the tip of the iceberg. For me, there are bigger WTFs than those:

    • It replaces non-printing characters with nul, effectively terminating the string at the first one, but in a way that implies the dev. thought they were simply stripping them out.
    • It both operates in-place and converts the result to a new type. If you're going to modify the caller's data, you may as well let them decide whether they want a std::string or not after.
    • It takes the length of the input as a parameter, which implies that it doesn't need a nul-terminated string... except it does, because it doesn't pass the length to the string constructor at the end.
  • (nodebb)

    Uppercase characters were used first in computing and therefore are smaller numbers in ASCII. At least that;s my theory and it helps me remember that subtracting 32 is a toUpper operation.

    I would 'optimize' the code by replacing the assignment with p_param[i] &= 0b1011111;

  • Debra (unregistered)

    TRWTF is the fact that it modifies its input argument, of course.

  • Hanzito (unregistered)

    It not more toUpperUntilNonPrintable(), because it also terminates the string on a non-printable.

  • (author)

    In my defense, I forgot how subtraction worked for a moment and misread it as adding 32.

  • LZ79LRU (unregistered)

    This is what we would call a Fractal WTF. Or WTF2 (squared) for short.

    You can peal its layers away like an onion. But no matter how deep you go the tears keep coming.

  • Argle (unregistered)

    Maybe "why?" should be removed from a maintenance programmer's vocabulary. It only leads to drug abuse, alcoholism or voting Libertarian.

  • (nodebb) in reply to Hanzito

    Rather inPlaceToUpperAndReturnUntilNonPrintable(), since this function de facto has two outputs.

  • David Brown (unregistered)

    Aside from the more obvious things wrong, just look at the arguments. Both have a sort-of Hungarian prefix on them. That leaves the first argument as "param", which tells us nothing about the purpose of the argument. The second argument also has a "p_" prefix, despite the length parameter being an int, and not a pointer.

  • a cow (not a robot) (unregistered)
    Comment held for moderation.
  • cellocgw and I am logged... or not (unregistered)
    Comment held for moderation.
  • LCrawford (unregistered)
    Comment held for moderation.
  • (nodebb) in reply to David Brown

    I'm guessing that this version of Hungarian decorates with "p_" to mean "parameter", which I find absolutely hilarious.

  • Andrew Klossner (unregistered) in reply to Rick

    "I would 'optimize' the code by replacing the assignment with p_param[i] &= 0b1011111;"

    This would require another load of p_param[i] that's already in variable j which is probably assigned to a CPU register.

    Also, bitwise AND is no improvement over subtraction. I've worked on CPUs that had two arithmetic units but only one AND/OR/NOT unit, so it could execute two subtractions in a cycle but only one AND.

  • (nodebb) in reply to Argle

    It would be more useful to remove "why not?".

  • Erik (unregistered) in reply to Debra

    If it's not declared 'const', then that's acceptable and should be expected. Maintaining const-correctness in a codebase is something that in my experience is sorely overlooked.

  • Erik (unregistered) in reply to David Brown

    This is something that I first encountered developing for Symbian (or EPOC as it was at the time) - https://silo.tips/download/symbian-os-c-coding-standards

    It's not Hungarian (which I dislike) - it only takes one person to change the declared type of something without changing all of the references to the name and now the compiler and the maintenance programmer are reading very different things.

    Instead, it's a scope indicator - in Symbian, "aThing" is an argument to a function, "iThing" is a member variable in the object instance, and there were lots more (so, "iThing = aThing;" was obviously just copying the function argument to the member variable without thinking of a different name - which makes constructors very easy to grok. In C++ where 'foo' might be a local variable, argument, instance variable, something in an active namespace etc, this actually made things much more understandable.

    The upper-case version is a more of a "capability" thing. Functions which could do the equivalent of throwing an exception (Symbian had a thing called a "Cleanup Stack" because C++ exception handling wasn't very well supported at the time) started with a 'C' - "CMyFunction". So, if you had a function that didn't start with a C but called functions which did, that was an obvious bug right there ("I'm calling things that can throw an exception, but my name does not indicate that I will throw an exception"). A fairly elegant solution at the time to an issue that is mostly handled by more capable tools these days.

  • Erik (unregistered)

    "So, intToString takes a buffer of bytes and a length for the buffer, and at a glance, you'd just assume that someone is reinventing atoi."

    Given that the name is "intToString" and the return type is String, I would not assume that this is atoi (or "stringToInt") :D

  • (nodebb) in reply to shard013

    Unless you're running it on an IBM mainframe, in which case its a lossy monoalphabetic substitution cipher function that swaps EBCDIC values around.

  • (nodebb) in reply to Andrew Klossner

    Should I have used double quotes instead of single quotes around the word 'optimize' so it would have been more apparent?

  • (nodebb) in reply to Erik

    It's not Hungarian (which I dislike) - it only takes one person to change the declared type of something without changing all of the references to the name and now the compiler and the maintenance programmer are reading very different things

    People keep forgetting the original intent of Hungarian notation, what is now called "Application Hungarian" (as opposed to "Systems Hungarian").

    To remind everyone...

    Systems Hungarian is the thing everyone thinks that "Hungarian notation" means, where the wart indicates the compiler's view of the type (i for int, f for float, etc.), and is rightly criticised for what you cite, among other things.

    Application Hungarian is Charles Petzold's (the Hungarian in question) original vision, where the wart indicates the "flavour" of the data in the variable, to make up for deficiencies in the language's type system. So for string variables (e.g. char * in C - remember, the type system is deficient) might have one of the following warts:

    • html == HTML-encoded text
    • plain == plain text, not HTML-encoded or whatever
    • cipher == encrypted ciphertext
    • etc.

    And correct calls to conversion functions resemble:

    • html_new_page = html_from_plain(plain_new_page); => converts plain text containing & and < and > and other ugliness into the HTML-encoded version
    • plain_received_data = plain_from_cipher(cipher_received_data); => decrypts the ciphertext

    And wrong calls to conversion functions resemble:

    • plain_new_page = html_from_plain(cipher_data); => "plain" doesn't match "html" in the assignment, and "plain" doesn't match "cipher" in the argument.

    It's largely obsolete these days in fully type-enforcing languages, since we use more elaborate type systems (C++, Cflat, Java, etc.) to have the compiler rather than the reviewer's eye enforce the right calls.

    Systems Hungarian is an abomination no matter which language you're using.

    The Windows API is littered with examples of both, of course.

  • (nodebb) in reply to Erik

    Given that the name is "intToString" and the return type is String, I would not assume that this is atoi

    Indeed, I missed that, too. It should be itoa.

  • Sole Purpose Of Visit (unregistered) in reply to Steve_The_Cynic

    Small correction: Charles Simonyi.

  • Erik (unregistered) in reply to Steve_The_Cynic

    Interesting - thank you for educating us :) It seems that Systems Hungarian is what I've been exposed to - and yes, it was the cause of totally avoidable bugs in Win32 code where someone checked in a change with a type/name mismatch and people later believed the name without checking the type.

  • Dave (unregistered)
    Comment held for moderation.
  • (nodebb) in reply to Sole Purpose Of Visit

    Ugh, yes. Thanks for the correction.

  • asynchronous (unregistered) in reply to Smithers

    Perhaps, he is thinking of PETSCII?

  • Derekwaisy (unregistered)
    Comment held for moderation.
  • Jimmyanten (unregistered)
    Comment held for moderation.
  • Musaran (unregistered)

    'i' is uninitialized because it is later set by the 'for'. 'j' is initialized despite being later set in the 'for'. Both should have been declared & initialized in said 'for'.

    A detail is never too small to sneak in inconsistencies and creep.

Leave a comment on “intToString”

Log In or post as a guest

Replying to comment #604208:

« Return to Article