2
\$\begingroup\$

Implement atoi to convert a string to an integer.

Requirements for atoi: The function first discards as many whitespace characters as necessary until the first non-whitespace character is found. Then, starting from this character, takes an optional initial plus or minus sign followed by as many numerical digits as possible, and interprets them as a numerical value.

The string can contain additional characters after those that form the integral number, which are ignored and have no effect on the behavior of this function.

If the first sequence of non-whitespace characters in str is not a valid integral number, or if no such sequence exists because either str is empty or it contains only whitespace characters, no conversion is performed.

If no valid conversion could be performed, a zero value is returned. If the correct value is out of the range of representable values, INT_MAX (2147483647) or INT_MIN (-2147483648) is returned.

The following is my code:

int atoi(const char *str) {
    int sign = 1;
    long long i = 0, j = 0;

    while ((*str) != '\0' && isspace(*str)) {
        ++str;
    }

    if (((*str)!='\0') && ((*str) == '+' || (*str) == '-')) {
        if ((*str) == '-') {
            sign = -1;
        }
        ++str;
    }

    if (((*str) != '\0') && (!isdigit(*str))) {
        return 0;
    }

    while (((*str) != '\0') && (isdigit(*str))) {
        i = i * 10 + (*str - '0');
        j = i * sign;
        cout << j << endl;
        if (j > INT_MAX) {
            return INT_MAX;
        }
        if (j < INT_MIN) {
            return INT_MIN;
        }

        ++str;
    }

    return j;
}
\$\endgroup\$

3 Answers 3

2
\$\begingroup\$
if (((*str) != '\0') && (!isdigit(*str))) {
    return 0;
}

You don't need this condition, because of the condition in the while loop afterwards. If that fails the first time j with value 0 is returned anyway.

And a minimal (and maybe unnecessary) optimization:

if (j >= INT_MAX) {
    return INT_MAX;
}
if (j <= INT_MIN) {
    return INT_MIN;
}

Why look for the next character in the string which would only get appended to the number and would increase the value, if the number already has the maximumk/minimum value possible (INT_MAX/INT_MIN).

\$\endgroup\$
5
  • \$\begingroup\$ Why look for the next character which only makes the number bigger if you already reached INT_MAX/INT_MIN? Can you explain this sentence more detail. I don't understand. \$\endgroup\$
    – Fihop
    Mar 20, 2013 at 18:32
  • \$\begingroup\$ @FihopZz, I edited my answer, I hope its understandable now (English is not my native language) \$\endgroup\$ Mar 20, 2013 at 19:17
  • \$\begingroup\$ If the number already has the maximum/minimum value, which is checked by if (j >= INT_MAX) and if (j <= INT_MIN) in my code, there is no necessary to check the next character. It seems that this is what I do. :-). \$\endgroup\$
    – Fihop
    Mar 20, 2013 at 22:40
  • \$\begingroup\$ In your code you return if j > INT_MAX, but you don't return if j is already equal to INT_MAX, so better IMO would be returning if j >= INT_MAX like I suggested. \$\endgroup\$ Mar 21, 2013 at 6:48
  • \$\begingroup\$ if (j >= INT_MAX) tests it too late to prevent undefined behavior of the prior i = i * 10 + (*str - '0'); on platforms where INT_MAX == LLONG_MAX. \$\endgroup\$ Feb 8, 2019 at 11:23
4
\$\begingroup\$

You seem to have too many checks for '\0'. They don't add anything. Also too many brackets, but better too many than too few.

Also, is long long guaranteed to be bigger than int? I don't think it is. Here's another version that doesn't need a bigger representation than that of int:

int atoi(const char *str)
{
    int n = 0;
    int sign = 1;

    while (isspace(*str)) {
        ++str;
    }
    if (*str == '-') {
        sign = -1;
        ++str;
    } else if (*str == '+') {
        sign = 1;
        ++str;
    }
    while (isdigit(*str)) {
        if (n > INT_MAX/10) { /* EDIT: protect against overflow */
            break;
        }
        n *= 10;
        int ch = *str - '0';

        if (n > INT_MAX - ch) {
            break;
        }
        n += ch;
        ++str;
    }
    if (isdigit(*str)) {
        return sign == 1 ? INT_MAX : INT_MIN;
    }
    return sign * n;
}

EDIT: I added a check to prevent overflow of the n *= 10

EDIT2: improved to avoid using an unsigned int - it was a hack

\$\endgroup\$
3
  • \$\begingroup\$ very nice!!!!!!! \$\endgroup\$
    – Fihop
    Mar 20, 2013 at 19:01
  • \$\begingroup\$ Minor is...(*str) is UB when *str < 0. Consider isspace((unsigned char) *str), isdigit((unsigned char) *str) . \$\endgroup\$ Feb 8, 2019 at 11:33
  • \$\begingroup\$ This code has a novel and functionally correct way to prevent overflow. Clarity to add: if (isdigit(*str)) { is true in 2 conditions: overflow and when forming a non-overflow result of INT_MIN. e.g. atoi("-2147483648"). \$\endgroup\$ Feb 8, 2019 at 11:36
1
\$\begingroup\$

is...() UB

is...(c) can be undefined behavior (UB) when c < 0. To prevent, cast to unsigned char.

// isspace(*str);
isspace((unsigned char) *str);

// isdigit(*str);
isdigit((unsigned char) *str);

Invalid C code

cout << j << endl; is not well defined in C. I suspect OP is using a non-C compiler.

Code assumes long long wider than int

if (j > INT_MAX) is not possible when INT_MAX == LLONG_MAX. Prior i = i * 10 can readily overflow signed integer math which is UB.

In any case, the approach of needing wider math fails when trying to code atoll().

Better to detect potential overflow using int math. Alternative:

int neg_sum = 0;
while (isdigit((unsigned char) *str))) {
  int digit = *str++ - '0';
  if (neg_sum <= INT_MIN/10 && (neg_sum < INT_MIN/10 || digit > -(INT_MIN%10))) {
    neg_sum = INT_MIN; // out of range
    break;
  } 
  neg_sum = neg_sum*10 - digit;
}

if (sign == 1) {
  if (neg_sum < -INT_MAX) {
    return INT_MAX; 
  }
  return -neg_sum; 
}
return neg_sum; 
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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