5
\$\begingroup\$

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 < eval; l++) {
            (esign >= 0) ? (rtn *= 10) : (rtn /= 10);
        }
    }

    // Finally return the solution
    return rtn;
}
\$\endgroup\$
6
\$\begingroup\$
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 < eval; l++) {
            (esign >= 0) ? (rtn *= 10) : (rtn /= 10);
        }

I suspect using a pow() function would be more fruitful.

    }

    // Finally return the solution
    return rtn;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.