jake1138 wrote:[color=blue]
> Here is a function I have to get a number at the end of a string. I'm
> posting this in case it proves helpful to someone. Comments are
> welcome.
>
> int getnum(char *str)[/color]
`const char *str' would be a little better, as it
indicates your intention not to modify the string.
[color=blue]
> {
> char buffer[BUFSIZE];
> char *buf = buffer;[/color]
I can't see any reason to make an extra copy of the
digits; you could just as well convert them directly
from the `str' string (but see below). Why bother with
the copy, especially since it places an artificial limit
on the length of the string you can handle?
[color=blue]
> int i;
>
> for (i = 0; i < BUFSIZE-1; i++) {
> if (str[i] == '\0') break;
> if (!isdigit(str[i])) continue;[/color]
Make that `isdigit((unsigned char)str[i])' to defend
against negative-valued character codes in the string.
[color=blue]
> *(buf++) = str[i];[/color]
Harmless but unnecessary and funny-looking parentheses.
[color=blue]
> }
> *buf = '\0';[/color]
This loop doesn't quite match your "number at the end
of a string" description. For example, consider what it
does with a string like "abc123xyz456". If you actually
intend to extract "123456" from such an input, then the
copy into buffer[] may be justified -- but if you intend
to convert just the "456" the copy is unnecessary and you
need to change the way this loop works.
[color=blue]
> if (buffer[0] != '\0') {
> return atoi(buffer);[/color]
Despite its name, atoi() is not a good choice of a
conversion function. The biggest drawback is that it has
no way to report errors in the input (in fact, erroneous
input provokes undefined behavior). And yes: a string
consisting entirely of digits can be erroneous; consider
the string consisting of one million '9' digits ... Try
strtol() instead, so you can check for errors after the
conversion. Alternatively, do the conversion yourself,
one digit at a time -- this may actually be easier.
[color=blue]
> } else {
> return -1;[/color]
I was also going to remark on the fact that you've
ignored the possibility of a minus sign before the digit
string, but this `return' makes me think you probably don't
intend to process a sign: "abc-123" is thought of as "abc-"
plus "123", not as "abc" plus "-123". That's fine, but you
ought to say so explicitly in your description of what the
function is supposed to do.
[color=blue]
> }
> }[/color]
Putting all this together leads to something like
(untested):
#include <stdlib.h>
#include <ctype.h>
#include <limits.h>
#include <errno.h>
int getnum(const char *str) {
const char *ptr;
char *end;
int dseen;
long value;
/* Advance `str' to start of all-digits suffix.
* (Alternating strspn() and strcspn() calls might
* be slicker, but this loop has the virtue of
* simplicity.)
*/
dseen = 0;
for (ptr = str; *ptr != '\0'; ++ptr) {
if (isdigit((unsigned char)*ptr)) {
if (dseen == 0) {
/* Digit string starts here */
str = ptr;
dseen = 1;
}
}
else {
/* Non-digit: suffix not found yet */
dseen = 0;
}
}
/* Can't proceed if no such suffix found */
if (dseen == 0)
return -1;
/* Try to convert the suffix */
errno = 0;
value = strtol(str, &end, 10);
/* Fail if unable to convert or if result is
* too large for an `int'
*/
if (errno == ERANGE || end == str || value > INT_MAX)
return -1;
/* Huzzah! */
return value;
}
--
Eric.Sosman@sun.com 
