Checking the validity of a string representing a number

Question

I need to write a function which will check a string for a few properties:

1. The string must represent a positive integer (> 0)
2. The integer mustn't require more than 32 bits of memory
3. There are no letters in the string

If these conditions are met, it should return the string as an int, if any of these conditions are not met, it should return -1.

Currently the function fails to deal with the following 2 inputs:

• 4y
• 13.4

If my isDigit() loop works as intended it'd be able to check for them. Why does the loop not work?

int convert(const char length[]) {
  long input = atol(length);
  if (input >= 2147483648 || input <= 0) {
    return -1;
  }
  int chkr = 0;
  while (chkr < strlen(length)) {
    if (isdigit(length[chkr++]) == 0) {
      return -1;
   }
    else {
      return atoi(length);
    }
  }
  input = atol(length);
  if (length[0] == '0') {
    return -1;
  }
  if (strlen(length) < 3) {
    return -1;
  }
 else {
    return atoi(len gth);
  }
}

Answer 1

Your function is terribly convoluted and wrong.

Use this instead and let the C library do the dirty work:

#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <ctype.h>

// The function you're interested in

int convert(const char string[]) {
  char *endptr;
  if (!isdigit((unsigned char)string[0]))
    return -1;

  errno = 0;    // need to set errno to 0 (see errno documentation)
  long value = strtol(string, &endptr, 10);
  if (errno != 0 || value <= 0 || value > 2147483647 || *endptr != 0)
  {
    return -1;
  }
  return value;
}

int main() {
  // Test different cases:

  struct {
    const char *input;
    int expected;
  } testcases[] =
  {
    // OK cases
    "123", 123,
    "1234", 1234,
    "2147483647", 2147483647,

    // fail cases
    "-1234", -1,      // number is negatif
    "12.3", -1,       // contains non digit '.'
    "123y", -1,       // contains non digit 'y'
    "2147483648", -1, // out of range
    " 123", -1,      // starts with a space

    // wrong test case on purpose
    "1234", 1245,
  };

  // Test all test cases

  for (int i = 0; i < sizeof(testcases) / sizeof(testcases[0]); i++)
  {
    int value = convert(testcases[i].input);
    if (value != testcases[i].expected)
    {
      printf("convert(\"%s\") failed, returned value = %d, expected value = %d\n", testcases[i].input, value, testcases[i].expected);
    }
    else
    {
      printf("convert(\"%s\") passed\n", testcases[i].input);
    }
  }
  return 0;
}

The program prints every test case. The last test case is wrong on purpose.

The for loop loops through a number of test cases and for each test case that fails it prints the values involved.

Output:

convert("123") passed
convert("1234") passed
convert("2147483647") passed
convert("-1234") passed
convert("12.3") passed
convert("123y") passed
convert("2147483648") passed
convert("1234") failed, returned value = 1234, expected value = 1245

Answer 2

As mentioned before your while loop returns after the first iteration.

Use sscanf instead of atol or atoi. This is preferable as you can detect errors:

int convert(const char *length){
    int err, sz;
    unsigned rtn;
    /*%u reads an unsigned integer (mostly 32 bit) >= 0*/
    err = sscanf(length, "%u%n", &rtn, &sz);
    /*check reading error occured*/
    if(err == 0){
        return -1;
    }
    /*check if there is no whitespace/sign*/
    if(!isdigit(length[0])){
        return -1;
    }
    /*check if 0 < rtn <= INT_MAX*/
    if(rtn <= 0 || rtn > INT_MAX){
        return -1;
    }
    /*check everything got read*/
    /*=> no letters*/
    if(sz != strlen(length)){
        return -1;
    }
    return rtn;
}

Let's test it:

/*these fail*/
const char zero[] = "0";
const char spaceStart[] = " 84654";
const char spaceEnd[] = "84654 ";
const char negative[] = "-7869";
const char tooBig[] = "2147483648";
const char fitsInto32BitInt[] = "2147483647";
const char positive[] = "+7526";
const char withLetter[] = "4y";
const char withPoint[] = "13.4";
/*these work*/
const char one[] = "1";
const char fine[] = "746838";
const char fitsInto32BitInt[] = "2147483647";

Answer 3

int convert(const char length[]) {
    if (atol(length) >= 2147483648 || atol(length) <= 0)
        return -1;

    int chkr = 0;
    while (chkr < strlen (length)) {
        if (isdigit(length[chkr++]) == 0)
            return -1;
    }

    return atoi(length);
}

Edit: Fixed answer. No clue why I didn't use isdigit().
Was trying too hard to be clever and it backfired, I guess.

Answer 4

Check out this loop:

while (chkr < strlen(length)) {
    if (isdigit(length[chkr++]) == 0) {
        return -1;
    }
    else {
        return atoi(length);
    }
}

You're not going through the whole string, you're returning after the first char anyway. You don't want to return in the else case here, otherwise that's skipping the rest of your program. Just have this:

while (chkr < strlen(length)) {
    if (isdigit(length[chkr++]) == 0) {
        return -1;
    }
}

After that, this line here

input = atol(length);

Is not needed. Input had that value anyway. It's not causing any harm, though.

Answer 5

You are only checking the first character and immediately return after that.

input = atol(length);

is unreachable.