patterncMinor
Implementation of the atof() function
Viewed 0 times
implementationfunctiontheatof
Problem
I am looking for feedback on coding style. I know this function already exists, and I would never use my version over a well tested one. I am just practicing.
double Clib_atof(char s[])
{
double val, power, rtn;
int i, sign;
for (i = 0; isspace(s[i]); i++);
sign = (s[i] == '-') ? -1 : 1;
if (s[i] == '+' || s[i] == '-') ++i;
for (val = 0.0; isdigit(s[i]); ++i) {
val = 10.0 * val + (s[i] - '0');
}
if (s[i] == '.') {
++i;
}
for (power = 1.0; isdigit(s[i]); ++i) {
val = 10.0 * val + (s[i] - '0');
power *= 10.0;
}
rtn = (sign * val / (power));
// Next work on exponent
if (s[i] == 'e') {
int j, esign;
int eval = 0;
fprintf(stdout, "e found\n");
for (j = i + 1; isspace(s[j]); ++j);
esign = (s[j] == '-') ? -1 : 1;
if (s[j] == '+' || s[j] == '-') ++j;
for (; isdigit(s[j]); ++j) {
eval = 10 * eval + (s[j] - '0');
}
fprintf(stdout, "eval = %d\n", eval);
int l;
for (l = 0; l = 0) ? (rtn *= 10) : (rtn /= 10);
}
}
// Finally return the solution
return rtn;
}Solution
double Clib_atof(char s[])
{
double val, power, rtn;
int i, sign;
for (i = 0; isspace(s[i]); i++);Looks too much like the
; was an accident. I suggest using {/\empty\/} instead to make it clear you did it on purpose. Or I might write it as a while loop.sign = (s[i] == '-') ? -1 : 1;
if (s[i] == '+' || s[i] == '-') ++i;
for (val = 0.0; isdigit(s[i]); ++i) {
val = 10.0 * val + (s[i] - '0');
}I dislike the use of a for loop here. The initializer condition doesn't really have any to do with the loop control. Repeating the loop until some condition becomes true feels like a
while loop. With a for loop I think of iterating or counting not testing.if (s[i] == '.') {
++i;
}
for (power = 1.0; isdigit(s[i]); ++i) {
val = 10.0 * val + (s[i] - '0');
power *= 10.0;
}
rtn = (sign * val / (power));I wouldn't use abbreviations like
rtn and val. They make the code harder to read. I don't think any of those parens are necessary. // Next work on exponent
if (s[i] == 'e') {
int j, esign;
int eval = 0;I look at this and think evaluate, but I don't think that is what you meant.
fprintf(stdout, "e found\n");
for (j = i + 1; isspace(s[j]); ++j);Why would you allow space here? Why
j? Why didn't you stick with i?esign = (s[j] == '-') ? -1 : 1;
if (s[j] == '+' || s[j] == '-') ++j;
for (; isdigit(s[j]); ++j) {
eval = 10 * eval + (s[j] - '0');
}I note that this is pretty much the same as a previous block of code. Consider extracting the common bits into a function:
fprintf(stdout, "eval = %d\n", eval);
int l;
for (l = 0; l = 0) ? (rtn *= 10) : (rtn /= 10);
}I suspect using a
pow() function would be more fruitful.}
// Finally return the solution
return rtn;
}Code Snippets
double Clib_atof(char s[])
{
double val, power, rtn;
int i, sign;
for (i = 0; isspace(s[i]); i++);sign = (s[i] == '-') ? -1 : 1;
if (s[i] == '+' || s[i] == '-') ++i;
for (val = 0.0; isdigit(s[i]); ++i) {
val = 10.0 * val + (s[i] - '0');
}if (s[i] == '.') {
++i;
}
for (power = 1.0; isdigit(s[i]); ++i) {
val = 10.0 * val + (s[i] - '0');
power *= 10.0;
}
rtn = (sign * val / (power));// Next work on exponent
if (s[i] == 'e') {
int j, esign;
int eval = 0;fprintf(stdout, "e found\n");
for (j = i + 1; isspace(s[j]); ++j);Context
StackExchange Code Review Q#6370, answer score: 6
Revisions (0)
No revisions yet.