patterncModerate
Implementing atoi() in C
Viewed 0 times
implementingatoistackoverflow
Problem
I was given a school assignment to implement
atoi(). Please review my code.int my_atoi(char *ato)
{
int res;
int sign;
res = 0;
sign = 1;
while (*ato == ' ' || *ato == '\t' || *ato == '\n' || *ato == '\f' || *ato == '\r')
ato++;
if (*ato == '-')
sign = -1;
if (*ato == '-' || *ato == '+')
ato++;
while (*ato >= '0' && *ato <= '9')
{
res = res * 10 + *ato - '0';
ato++;
}
return (sign * res);
}Solution
Be consistent with
The standard declaration is
What happens if a NULL pointer is passed?
As written, you dereference
Edit: Martin R points out that silently returning 0 when a NULL pointer is passed to your function, as I have suggested, is bad programming practice, because it hides the error. If there's a possibility of calling
No need for separate variable definition and initialization
Do:
Don't:
Don't reinvent the wheel
Your first loop that skips over whitespace could use the standard library function
Edit: spot the bug in my original code... :/
The condition check in the second loop can use the stdlib function
If your instructor or assignment instructions prevent you from using
* Except you have a minor bug in your check for space characters. You also need to check for vertical tab (
Edit: Chux points out that calling
The correct usage, then, of the
Chux also pointed out that your original code as written did not have this problem; you weren't invoking undefined behavior. Moral of the story: be careful of the advice you receive from strangers on the Internet...? =)
Other than the issues mentioned, you have a decent implementation!
stdlib.h declaration of atoi()The standard declaration is
int atoi(const char str);. Your declaration should match (i.e., take a const char instead of char *).What happens if a NULL pointer is passed?
As written, you dereference
ato without checking if it is NULL (0). That is a big no-no in C. At the very top of the function, you should check if it's 0 and return 0 if it is:if (!ato) { return 0; }Edit: Martin R points out that silently returning 0 when a NULL pointer is passed to your function, as I have suggested, is bad programming practice, because it hides the error. If there's a possibility of calling
myatoi() with a NULL pointer, there's no way for the function to notify the caller of the error. Therefore, it's incumbent upon the caller to check before calling. No need for separate variable definition and initialization
Do:
int res = 0;
int sign = 0;Don't:
int res;
int sign;
res = 0;
sign = 1;Don't reinvent the wheel
Your first loop that skips over whitespace could use the standard library function
isspace():Edit: spot the bug in my original code... :/
Buggy (oops): while (isspace(*ato++));
Correct: while (isspace(*ato)) ato++;The condition check in the second loop can use the stdlib function
isdigit():while (isdigit(*ato)) {
...
ato++;
}If your instructor or assignment instructions prevent you from using
isspace() and isdigit(), why not go ahead and define them yourself? You already have the logic!** Except you have a minor bug in your check for space characters. You also need to check for vertical tab (
\v).Edit: Chux points out that calling
isspace(ato) and isdigit(ato) when *ato declares several functions useful for classifying and mapping characters. In all cases the argument is an int, the value of which shall be representable as an unsigned char or shall equal the value of the macro EOF. If the argument has any other value, the behavior is undefined.The correct usage, then, of the
isspace() and isdigit() functions in this code would be:while (isspace(*(unsigned char *)ato)) { atoi++; }
//...
while (isdigit(*(unsigned char *)ato)) {
//...
ato++;
}Chux also pointed out that your original code as written did not have this problem; you weren't invoking undefined behavior. Moral of the story: be careful of the advice you receive from strangers on the Internet...? =)
Other than the issues mentioned, you have a decent implementation!
Code Snippets
if (!ato) { return 0; }int res = 0;
int sign = 0;int res;
int sign;
res = 0;
sign = 1;Buggy (oops): while (isspace(*ato++));
Correct: while (isspace(*ato)) ato++;while (isdigit(*ato)) {
...
ato++;
}Context
StackExchange Code Review Q#131820, answer score: 11
Revisions (0)
No revisions yet.