String to Float

[taurous]

So I came up with a method to convert a float to a string:

Code: Select all

float stringToFloat(std::string str, float defaultVal = 0)
{
	float decimal = 0;
	int integer = 0;
	int sign = 1;

	for (int i = str.size()-1; i >= 0; i--)
	{
		int digit = (int)str[i] - 48;
		if (digit >= 0 && digit < 10)
		{
			decimal+=digit;
			if (decimal != 0)
				decimal/=10;
		}
		else if (str[i] == '.')
		{
			for (int j = 0; j < i; j++)
			{
				int digit = (int)str[j] - 48;
				if (digit >= 0 && digit < 10)
				{
					integer*=10;
					integer+=digit;
				}
				else if (str[j] == '-') sign = -1;
				else return defaultVal; // Remove this line to find a number in a string with characters.
			}
			return (decimal+(float)integer) * sign;
		}
		else return defaultVal; // Remove this line to find a number in a string with characters.
	}
	return defaultVal;// Remove this line to find a number in a string with characters.
}

I'm proud of it, one of the first things I've been able to figure out without using google. I've never had feedback on my programming, so I was hoping to get some here! Can you guys point out anything bad or unnecessary? (bad meaning things that you shouldn't do ...you know.. besides the whole thing...)

[MarauderIIC]

So I came up with a method to convert a float to a string:

Code: Select all

float stringToFloat(std::string str, float defaultVal = 0)
{
	float decimal = 0;
	int integer = 0;
	int sign = 1;

	for (int i = str.size()-1; i >= 0; i--)
	{
		int digit = (int)str[i] - 48;
		if (digit >= 0 && digit < 10)
		{
			decimal+=digit;
			if (decimal != 0)
				decimal/=10;
		}
		else if (str[i] == '.')
		{
			for (int j = 0; j < i; j++)
			{
				int digit = (int)str[j] - 48;
				if (digit >= 0 && digit < 10)
				{
					integer*=10;
					integer+=digit;
				}
				else if (str[j] == '-') sign = -1;
				else return defaultVal; // Remove this line to find a number in a string with characters.
			}
			return (decimal+(float)integer) * sign;
		}
		else return defaultVal; // Remove this line to find a number in a string with characters.
	}
	return defaultVal;// Remove this line to find a number in a string with characters.
}

I'm proud of it, one of the first things I've been able to figure out without using google. I've never had feedback on my programming, so I was hoping to get some here! Can you guys point out anything bad or unnecessary? (bad meaning things that you shouldn't do ...you know.. besides the whole thing...)

It's pretty good -- relatively straightforward, style's alright. Excellent for a beginner.

Style-wise: As a defensive coding practice, I always use curly braces, even if it's one line. In general, it makes maintenance and refactoring a little easier too.

Just looking at it, my codey-sense says that you could simplify it, though. Further examination reveals that you're doing the same check in two spots (if digit >= 0 && digit < 10), for instance, and throwing away a conversion (the first int digit = (int)str - 48), and once you hit the second loop you never come back to the first one; so surely there's some way to combine the two into a single loop to simplify the code. But my initial crack at this would be totally different anyway (see spoiler below). The values are pretty narrow. Can you use a signed character instead of an integer, and compare that against >= 0 or < 10? I never use signed or unsigned chars, really, so I'm not 100% on whether or not it would work ("char" doesn't have a defined type). I wouldn't do the "remove this line" stuff. At worst, I would add a parameter to this function and wrap those lines in if statements. But again, three statements that have the same purpose; something here can be simplified :)

Instead of returning a default value that the user can pass in, typical style is to return a special value on error (for instance, string returns string::npos when it can't find a character). In this case, I would return FLT_MAX (I find that things generally return MAX values rather than MINs on error if they have to).
http://www.cplusplus.com/reference/clibrary/cfloat/
Related:
http://www.cplusplus.com/reference/clibrary/climits/

My efforts
[spoiler]None of these are checked to see if they even compile, nor for correctness.

My first crack at it, but I don't like the error conditions on it.

Code: Select all

/* stringToFloat
@brief Converts a C++ string to a float by splitting at the decimal point
@param string str[in]    string to convert
@returns float
@return 0 if no conversion could be completed
@return the string as a float, e.g. "123.45" returns 123.45
*/
float stringToFloat(std::string str)
{
    float decimal;
    float integer;

    size_t point = str.find(".")
    string tmp = str.substr(0, point);
    integer = atoi(tmp.c_str()); /* if error, 0 is returned. yes, sucks. it's a c function, too. use stringstream? */
    if (point == std::string::npos) /* no decimal point. I really wanted to ternary operator here */
    {
        decimal = 0;
    }
    else
    {
        tmp = str.substr(point + 1);
        decimal = atoi(tmp.c_str());
        decimal /= (10 * tmp.length()); /* ... but then I realized that I would need the length of the # of digits behind the decimal point :( */
    }
    return (integer + decimal);
}

My second crack at it (remembered strtol when writing 'atoi', so started checking references)
http://www.cplusplus.com/reference/clib ... dlib/atoi/
http://www.cplusplus.com/reference/clib ... ib/strtod/

Code: Select all

float val = (float)strtod(str.c_str(), NULL);

Code: Select all

 char* err = NULL;
float val = (float)strtod(str.c_str(), &err);
if (val == 0.0 /* hmm checking for zero on floats is so bad, but it says that that's what's returned */ && err != str.c_str()[str.length()-1]) { /* error */ }

Then I started googling. http://stackoverflow.com/questions/5773 ... bers-in-it[/spoiler]

[taurous]

Thanks so much for the feedback! I'll take a crack at seeing how I can simplify it more.

EDIT:

I worked on simplifying and optimizing it and I came up with this:

Code: Select all

float stringToFloat(std::string str, bool ignoreLetters)
{
	float number = 0;
	int sign = 1;
	int decLength = 0;

	for (unsigned short i = 0; i < str.size(); i++)
	{
		int digit = (int)str[i] - 48;
		
		if (digit >= 0 && digit < 10)
		{
			number*=10;
			number+=digit;
		}
		if (str[0] == '-')
		{
			sign = -1;
		}
		else if (str[i] == '.')
		{
			decLength = str.size()-1-i; // length of decimal
		}
		else if (!ignoreLetters)
		{
			return FLT_MAX; // non number encountered
		}
		else if (ignoreLetters && decLength != 0)
		{
			decLength--; // shortens the decimal length if letters are found in the decimal.
		}
	}

	return (number/(pow((float)10.0, decLength))) * sign;
}

Instead of looping left to right the the left half of the float, then looping right to left through the right half and adding them together, I now loop through the whole number. It puts all the digits from the string into an integer, then divides by the length of the decimal. The length of the decimal is found by counting how many characters are left after encountering the '.'. Less looping and more control! It will even work if there is no decimal!

I just read your latest post, I also didn't see your attempt at this before I changed it. I found out how to find if there are characters in a string once I figured out that the character '0' is 48. so if the char-48 is not between 0 and 9 it's a character. I assume this method doesn't work with different character sets, so it's probably a bad approach.

[MarauderIIC]

Thanks so much for the feedback!

You're welcome.

Code: Select all

		if (str[0] == '-')
		{
			sign = -1;
		}

You can pull this out of your loop.

I just read your latest post, I also didn't see your attempt at this before I changed it. I found out how to find if there are characters in a string once I figured out that the character '0' is 48. so if the char-48 is not between 0 and 9 it's a character.

If you're responding to what I think you're responding to... I meant, instead of using an int for digit, use a signed char. A pointless optimization, but it's one byte instead of 4 or 8.

I assume this method doesn't work with different character sets, so it's probably a bad approach.

You'd be surprised at how often that that isn't something I worry about :)

Once you pull out the str[0] check from your loop, if I saw this piece of code in a program, I probably would leave it alone... as long as it worked, heh heh.
But since you're asking for feedback...
The return statement draws my eyes and I assume it works fine. However -- maybe you should stop multiplying once you hit the decimal, because glancing at it, you multiply and then you undo some of the multiplications later? I didn't trace it, so correct me if I'm wrong.