Feedback wanted for a C function

Attention fellow Grasshoppers, I need your input on a C function I wrote. As you may know, I’ve recently decided to give C a serious look. The first project I decided to write is a simple RPN calculator. Last night, I wrote a function to verify if a string is a valid number representation; I wrote a few test cases using assert() and it seems to work pretty well (speaking of testing, do you have any recommendations for testing frameworks/libraries for C?) I do find the code long however, and the last loop seems complicated. Here it is (let’s hope Wordpress doesn’t mess up the formatting):


int isnumber(char *s)
{
    size_t i;
    size_t len;
    int period = 0;

    /* Skip over white spaces. */
    while (isspace(*s))
        ++s;

    if (*s == '+' || *s == '-')
        s++;

    i = 0;
    len = strlen(s);

    if (len == 0)
        return 0;

    for (i = 0; i < len; ++i) {
        /* Only one period is allowed. */
        if (!period && s[i] == '.') {
            period++;
        }
        else if (period && s[i] == '.') {
            return 0;
        }
        /* Ignore everything after a space. */
        else if (isspace(s[i])) {
            return 1;
        }
        else if (!isdigit(s[i])) {
            return 0;
        }
    }

    return 1;
}

Let me know what you think and what could be improved. I’ll appreciate any input, whether it’s stylistic, functional, security-related, performance-related, etc.

2 Comments

  1. jamessan:

    Just as a matter of course, the first thing I’d do in such a function is:

    
    if (s == NULL || strlen(s) == 0)
        return 0;
    
    

    The inconsistency between your use of post-increment and pre-increment is a little odd. Other than that, it looks good.

  2. Jeremy Fincher:

    First, the function is too long. Write another function to skip leading whitespace (or, for extra credit, write a function to skip leading chars matching a certain predicate) and use that.

    Second, I wouldn’t use strlen at all. It doesn’t significantly simplify your function and it results in making two passes over the string rather than one. Instead, just use a while loop like “while (c = *s++)”.

    You could write it in a more state machine style, if you want, but it’s not necessary.

Leave a comment