CodeSOD: Padding the Amount

Matt's co-worker needed to handle some currency values coming in as strings. As this was in C++, we're already in a fraught territory of having to worry about whether the callers and various APIs we're using handle C-style strings or C++ std::string.

Or, you can just mix-and-match in code and hope for the best, while having some interesting approaches to handling your inputs. That's what this developer did.

int Amount(char *value,int len){ char *scanp = value; string s; float f; long l; if (strchr (scanp,'.')) { f = atof(value); f *= 100; f /= 100; l = (int)f; } else { // if the decimal was not found l = atol(value); } sprintf(value,"%li",l); s = value; while(s.length() < len) s.insert(0,"0"); if (s.length() > len) { char szTemp[256]; memset(szTemp,0,sizeof(szTemp)); strncpy(szTemp,s.c_str(),len); strcpy(value,szTemp); } else strcpy(value,s.c_str()); return 1;}

There's a lot going on here. The purpose of this function is to return the dollar amount of a currency value, padded out to a specific length. Right off the bat, we're in bad territory- mixing string and numeric operations in a single function with concerns around formatting.

But let's trace through.

First, we take our value pointer and create a scanp pointer which points to the same place. Then, we scan scanp for a decimal point. strchr treats its input pointer as const, so it isn't going to modify the input, it returns a pointer to the character in the string- or a null. So assuming there's a . in the string, we try and parse the data.

We call atof which converts a string to a float. Then we multiply it by 100, then we divide it by 100. Finally, we cast it to an int, discarding the decimal part.

Or, if there are no decimal points, we just use atol to parse the string into a long in the first place.

There's so much wrong about this. First off, the check for whether or not the string contains a decimal point is irrelevant. atof works just fine if there is no decimal point, and atol also works just fine if there is- it stops parsing at that point. This entire block could just be l = atol(value) and would have the same result.

And then we have the multiply and divide for no reason, which is a real head scratcher. I think they were trying to figure out how to round, but couldn't? The final bonus here is that, in the floating point branch, they cast the float to an int, instead of a long, which is the kind of thing you can get away with up until you can't. It's probably not going to cause them problems, but surprise truncations await if the inputs are sufficiently large.

Now that they've got their truncated value, they use sprintf to overwrite the input value data and replace it with the truncated value. At this point, it's worth noting that if you needed to do padding, your printf format strings can handle that for you, but that's not the approach here.

Instead, we create a std::string by assigning our C-style string to a string object. While the length of the string is less than our desired length, we pad it out with zeroes.

Now that it's padded, we check if the length of the string is longer than the maximum length we should return. Note: the only way this could happen is if the number has more digits than len, which means everything that follows is absolute nonsense.

They fill a temp array with zeroes, then copy our string into it, but only len characters. Then we copy the temp array back into value. If this branch ever executes, what it's actually doing is chopping off the trailing significant figures.

All in all, this is a terrible way to do everything that it does. Mixing C/C++ style idioms, mixing the logic for parsing, truncating, and formatting all in one method, it just ends up being a recipe for failure.

Or I guess, almost failure. After all, this function never fails- it always returns 1.

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!