- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
I've written a function that does this before. It has to process one digit at a time instead of going 1,000 at a time, otherwise you end up with "1,23" instead of "1,023"
Admin
I don't know... What's so horrible about this?
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?
Admin
Not sure this is a WTF really
it work and doesn't seem too ugly...
Admin
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.)
Admin
And what's "goofy" about modulus math?
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?
Admin
C doesn't have number_format() ?
Admin
looks standard fare for an embedded firmware engineer.
Admin
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.
Admin
Seems OK to me.
Variable names could be a little better perhaps, and you should really comment something like this, but I don't see what's wrong with it.
One improvement that I would probably make is to make the loop infinite with an if(!n) break before adding the comma as this means that you don't have to remove it afterwards.
Admin
Apart from global variable and possible buffer overflow (already pointed out), this looks pretty ok.
Assuming there are no library functions and you really have to code a function to format text, I challenge the OP to come up with something better, more elegant and/or more efficient.
Admin
A function that has variables?? Shocking! :-P
Admin
The real WTF is people thinking that having no comments, pointless and not so helpful variable names and a stupid function name is not a WTF.
Also, isn't there any number format C function??
Admin
Looks fine to me. Give it a long and you get out a nicely formatted user-friendly string in a buffer. Apart from the concurrency/overflow issues mentioned, this is fine.
Admin
No, it does not have to process one digit at a time. You just check if the remainder is below 10 or 100 to add leading "00" or "0".
But it guess it's the same either way.
Admin
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.
Pascal
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.
Admin
I'll give a WTF for using what appears to be a static buffer and assuming a long will fit a char[20]. 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.
Admin
Contrary to what I just said, there is indeed a risk of buffer overflow even with just 64 bits for longs. I forgot to count the minus sign and all the commas.
Pascal
Admin
True, but that introduces another edge case to avoid "001,023" That leads to more instructions, including branches. Granted we are only talking about a few cycles difference.
Admin
It's C (not C++) -- this just is what the language looks like. Aside from "minor" issues with static-length buffer and stuff, I don't think it could be made any better while still keeping it C.
Admin
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.
Admin
The real WTF is assuming that all locales use "," for their thousands separator. "1.000.000" is common for a million in Europe.
Admin
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...
Admin
Hell yes, I've written dozens of similar functions. When string conversion/formatting is not available (due to memory constraints) it's the only way to go. No WTF here.
On the other hand, I have also spent many long hours debugging such functions. I use printf and friends instead of home-cooked conversion whenever possible.
Admin
Submitter does not know any C if they think that is a WTF.
Admin
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.
Admin
Maybe the buffer is actually larger than 20. That's a fairly common kludge. It would've been nice to see a "//20 is to keep the results inside a char[27]"
The use of an external buffer is always a pain but I'd rather see that than a pointer to a local scope variable -- that's a real nightmare waiting to bite.
As for localization... many applications simply don't need it.
Admin
Remind me again just how you get commas in numbers using standard printf?
Admin
Err, scratch my buffsize being different than buffer size comment... just waking up.
Admin
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)
Admin
Wrong brace style! WTFFFFF!
Admin
I'm going to have to agree with the masses - seems fair enough to me.
Admin
printf("%'d",number);
Admin
Programmer mean_programmer (Programmer programmer_list[], int count) { Programmer all_programmers; for (int i = 0; i < count; ++ i) all_programmers += programmer_list[i]; return all_programmers/count; }
Admin
Other than the mysterious pre-allocated buffer and the assumption of maximum size this function is very good. In fact I've saved it as useful snippet.
Admin
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!
Admin
Yes, dot is the standard in continental Europe (which uses a comma instead of a decimal point), but the comma is used in the UK.
Admin
We'll assume that the default constructor for Programmer initialises it to 0, but other than that, you forgot to handle the case when count equals 0.
Admin
Admin
I agree, not much of a wtf here. In C++ (although I don't see anything indicating this is C++ and not C), however, this is a better way to do it portably:
std::stringstream ss; std::string formatted; ss.imbue(std::locale("")); ss << num; formatted = ss.str();Admin
I'm assuming it's on some embedded platform with limited library support.
Anyway, looks completely fine to me, and it really only takes a couple of seconds to figure out what it's trying to do.
The function name is a little silly, maybe nice_number_format() instead.
Admin
That right there is TRWTF. =p
Admin
I do.
Admin
I got the meaning of the function on my first read, and I'm not even a C programmer. Ergo, comments would be redundant here.
I'd change the name (a little) to pretty_print, or something more meaningful, and solve the few minor defects the function has, but that's it.
I think we're way past the "uses foo as a name function" WTF. If we're back to posting stuff because of bad naming conventions, plus the ban on TopCoder, this site is doomed.
Admin
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.
Admin
Admin
the real wtf is this codesod
captcha:tristique - it's how i'm feeling looking at this snippet
Admin
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[20];
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?
Admin
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: "-2,305,843,009,213,693,952\0" which is 27 characters long.
Admin
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.
In short, nice_function, mean_Vinson.
Admin
[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.