This way, you can get right-aligned output quite neatly. Add sufficient amount of spaces in front of first digit (or sign) and that's it, just make sure you keep it to the allocated buffer. There's virtually no overhead compared to string concatenation.
And dividing by 1000... You have to divide by 10 anyway to produce the individual digits. Additional divisions by 1000 would be reduntant and slower.
It's optimized for speed, obviously, and yet, at least to me, program flow was easy to follow. So, where's the WTF?
I would think you could use some string formatting to pad each section with 0s. But in any case that's just an efficiency question.
There's nothing in this function that prevents the pointer from going lower than its original value. Also, it uses what is apparently a global array as the actual destination, and just assumes the size is at least 20. So, there is a possibility of overwriting memory on both ends. (Unless a long value can't get big enough that it would fill more than 20 digits, it's too early for me to think about that though.)
I know of two ways to implement this function. One is like the code presented here, the other way is to use sprintf and adjust the results. You still have to deal with edge cases there to make sure you don't end up with "-,123"
Is it just me, or is the real WTF why this got posted as a CodeSOD?
Um, this function is fairly clean, blazing fast, and reasonably easy to understand for a competent C programmer. It's not thread-safe in the least, and it has buffer-overflow issues if you try to port the code straight into a 64-bit environment (maximum string length of 20), but calling this a "WTF" is a stretch. Other than fixing these two problems, I would be hard pressed to write a more elegant method for solving this problem.
"I don't know the language very well and therefore code examples written in it are hard to read" doth not a WTF make.
Apart from the comment saying that this code will break when 'long' is strictly more than 64 bits and a meaningful function name, I do not see either what the submitter is complaining about. These is nothing "goofy" about dividing by 10 and taking the modulo to get to the digits.
Oh, one more thing: the function breaks for MIN_LONG because the instruction
n = -n; overflows in this one and only case. Since it is only one case and
if the size of long is know, the simplest fix is to have a static literal string
for MIN_LONG and to test for this value.
Somehow, I think that this is not the reason the submitter sent this function.
The value analysis in Frama-C (http://frama-c.cea.fr) finds this kind of overflows,
but then it emits false alarms too.
I'll give a WTF for using what appears to be a static buffer and assuming a long will fit a char. Other than that, it looks reasonable. Hopefully the compiler is smart enough notice it can accomplish the mod and the divide with one instruction on most processors.
There are a couple of things here that would raise a flag if I was doing a formal code review (like the handling of edge cases, and the use of a global buffer that may not be big enough), but the approach as a whole is sound. It's clever, it's efficient, and honestly it should be pretty straightforward to understand for any C programmer. I knew what it was doing, and how it was doing it, after one read, top to bottom, without having to backtrack. To me, that's decent code.
IF this is for an embedded/micro controller application(where using a library may not be worth it), its pretty OK, else... Its not nice to read, not entirely safe for all cases and in bad need of comments. Not sure if that's enough to say WTF, tho...
First, the code is quite clean and fast. Starting at the end is clever, but not so clever as to make it unmaintanable (what is great), and there is no need for coments, the code is obvious, it just need a better name, so people don't have to read it at all.
Now, there is no buffer overflow for a tipical 32 bits long. People around here thinking that long means a 64 bit number is, well, weard. Ok, there is no garantee that a long will have only 32 bits, but that was the standardized size. There is also no garantee that a long will have 64 bits, just 32.
If there is a WTF at all, it is that the developer didn't use some standard library. Now, if that is embebed code, he has plenty of reasons to both not use standard libs nor expect a long that has more than 32 bits.
I'm quite surprised by the line "Yeah, it takes a few minutes to figure out." It seems rather straigthforward to me. The size of the buffer (20) might prove a little short (because he adds ',' in the string), there should be a better solution to fetch the thousands separator, maybe there's some builtin format in sprintf that does this, or a first sprintf might prove more effecient, and then add the separators, I don't know, but that's some code I wouldn't be surprised to find in a commercial product... Frankly, unless it proves to be a perfomance bottleneck, I'd let such code in the code base (well, after renaming it, perhaps... something more along the lines of "format_long" or whatever)
Remind me again just how you get commas in numbers using standard printf?
You use sprintf to get the number into a string, and then you simply add a comma every 3 spaces in some kind of loop... It takes all the "math" out of the equation and turns the problem into a simple string manipulation.
And for all of you asking "isn't there a library function to do that", the answer is NO. The C language is about getting stuff done -- it's not about reading a book or searhing on the internet for 10 minutes to find a library somewhere that does what you want to do. If I was faced with the comma problem, I would have done exactly what the OP(rogrammer) did.
And for everyone else talking about buffer overflows and global variables, etc., it might just be me, but these "advanced" problems DO NOT constitute a WTF. Chances are very good that this function would be used to convert a known range of numbers and none of your theoretical problems would ever come up. Is it the best coding practices? NO! It's not even very maintainable because of the globals and the "magic numbers", but for all we know, this snippet was from a small, simple program that was done quickly an efficiently.
To me, a WTF is something that simply makes no sense at all -- not trivial technical mistakes. We don't want this to be like that English nazi site with 1000's of posts making fun of signs that use "your" instead of "you're".
So, thumbs down to the WTF, and that's too bad because there have been some good ones lately!
Commas, periods, for thousands separators. How about the Canadian way, with spaces? All Canadian schoolchildren know about this. Although, once we grow up, we drop the pretentiousness and just do what the USians do.
looks standard fare for an embedded firmware engineer.
I bet this is the case. You don't always have all C library functions available; and you want everything to be pretty fast; and you can allow yourself some shortcuts (no overflow check), because you know the hardware.
I would really like to know what the wtf is. What exactly is the problem with this code? Comments? Comments are useless, given how straightforward the code is, and would detract from its clarity.
The largest possible string for a given input is "-2,147,483,648\0", which is 15 characters, so there is no buffer overflow. The function is static so is only used in one file. All that's missing is the external declaration
static char prtbuf;
and an example of usage:
printf("The number is %s\n", nice_num(foo));
which is the equivalent of
printf("The number is %i\n", foo);
except that it has thousand separators.
Given the local scope, the obviousness of its implementation, and the obviousness of its usage, what's wrong with the function name?
And how exactly are you supposed to convert binary to a decimal string without dividing by 10?
The largest possible string for a given input is "-2,147,483,648\0", which is 15 characters, so there is no buffer overflow.
I agree with you in everything but this sentence. The size of long may be anything, as long as it is at least as long as an integer. It may be several megabytes for all you'd know. In practice, it will be either 4 or 8 bytes. On my 64 bits processor, it's 8 bytes, which makes the largest possible string:
which is 27 characters long.
I'm going to have to join the chorus here: I don't see much of a WTF; especially the "few minutes to figure out" argument doesn't hold, for me "I came, I saw, I understood"; granted, I know C better than my own mother by now, but even then it's pretty understandable. It might be a bit too clever by half with predecrements, and I'd at least put the global string buffer size in a #define and of course give the function a better name, but otherwise I'd just comment its limitations (restricted to 32 bit, not thread-safe, locale (if applicable for context)) and move on. It even gets the '0' edge case right!
I guess everything depends on context, and if this was used in a web app it may be a WTF, but the mere fact it's written in raw ANSI C suggests a context with few luxuries in which this code makes perfect sense.
[quote user="JPhi"][quote user="GettinSadda"]
To me, a WTF is something that simply makes no sense at all -- not trivial technical mistakes. We don't want this to be like that English nazi site with 1000's of posts making fun of signs that use "your" instead of "you're".[/quote]
When you say "We", I think you mean "I" and when you say "1000's", I think you mean "1000s".
Normally I ignore any post that appears to be written by an illiterate child but the rest of yours was okay.