My first attempt at K&R 2-3's hex to integer htoi()

I'm working through K&R on my own to learn C.

2-3 Write the function htoi(s), which converts a string of hexadecimal digits (including an optional 0x or 0X) into it's equivalent integer value. The allowable digits are 0 through 9, a through f, and A through F.

I've opted to convert each valid digit to it's 0-15 equivalent and ignore non valid characters.

I tried to not use anything not presented in these first 46 pages.

I'm using static input until I get htoi() correct.

#include <stdio.h>
#include <ctype.h>
#include <string.h>

int htoi(char s[]);
// hex alpha char to integer
// 0..1 and Aa - fF
int hatoi(char c);

int main()
{
    char s[] = "0xfff";
    int res; /* result */
    res = htoi(s);
    printf("%s = %d\n",s,res);
    return 0;
}

int hatoi(char c)
{
  int res;
  if (isdigit(c))
    res = c - '0';
  else if (c >= 'a' && c <= 'f')
    /* offset a..f to 10-14 */
    res = (c - '0')-39;
  else if (c >= 'A' && c <= 'F')
    /* offset a..f to 10-14 */
    res = (c - '0')-7;
  else
    res = 0;
  return res;
}

int htoi(char s[])
{
  int len,i,result,power,digit;
  result = power = 0;

  len = strlen(s)-1;

  for (i=len; i >= 0; --i) {
    digit = hatoi(s[i]);
    if (digit) {
      if (power 开发者_如何学编程== 0) {
        result += digit;
        power++;
      } else
        result += digit * power;
      power *= 16;
    }
  }
  return result;
}

Am I close? It seems to work alright. I want to make sure I'm not learning bad habits and that I'm grasping what I should be by chapter 2.

Some thoughts:

• Your code will not work if your string contains a 0 in the middle; consider the test you're doing on the return value of hatoi.

• The numbers 39 and 7 seem a bit magic. It's clearer if you can derive them explicitly in code.

• It's good practice to always put code blocks after if or else in braces, even if it's just a single statement.

• Why not initialise power to be 1? That way, you don't need that special-case logic in your loop.

• I'd use int for hatoi() parameter: int hatoi(int ch);

• You have no way to distinguish between a valid '0' and an invalid character in hatoi().

• The htoi() function can be simplified quite a bit. For instance, the if (digit) test is unnecessary (you are looping from strlen-1 to the beginning).

Overall, this looks pretty good. I'll add some comments inline for the things I think you could improve.

int hatoi(char c)
{
  int res;
  if (isdigit(c))
    res = c - '0';

There's no real reason to create a res variable. You always return whatever you set in that variable, and never change it. Why not just replace res = c - '0' and the later return res with return c - 0?

  else if (c >= 'a' && c <= 'f')
    /* offset a..f to 10-14 */
    res = (c - '0')-39;

This seems somewhat convoluted. Why are you subtracting '0' and then 39? It would be much more clear to say (c - 'a') + 10. Also, the comment is wrong, it should say 10-15.

  result = power = 0;

  len = strlen(s)-1;

  for (i=len; i >= 0; --i) {

Your loop is running over the whole string; but in a hex string like 0xabcd, the 0x probably shouldn't be considered part of the number you are parsing. The way you are doing it, treating unknown characters as 0, it shouldn't matter for your test string, but if you other stuff at the beginning (like 1230xabcd), you would get a fairly strange result. I would recommend checking that the first two characters actually are 0x (probably returning 0 if not), and then looping down to 2 rather than down to 0.

    digit = hatoi(s[i]);
    if (digit) {

You appear to only be increasing power if the digit is non-zero. Thus, for a number like 0x0102, you would get 18, instead of the correct result 258. There's no need for the if (digit) check. If you want to return a sentinel from hatoi in the case of invalid characters in order to ignore them, I'd recommend returning -1, and then check if (digit >= 0).

      if (power == 0) {
        result += digit;
        power++;

If you initialize power to 1 instead of 0, you will not have to have this special case.

      } else
        result += digit * power;
      power *= 16;
    }
  }

Is isdigit limited to 0-9, or are they affected by locale? Wouldn't want the latter.

res = (c - '0')-39; should be res = (c - 'a')+10;

res = (c - '0')-7; should be res = (c - 'A')+10;

Note that this only works on ASCII-based machines. Numbers and/or letters aren't sequential on EBCDIC machines.

The argument should be a const pointer.

htoi is very complicated. You should find num = (num << 4) | digit; very useful.

int htoi(const char *s)
{
   int result = 0;
   while (*s)
      result = ( result << 4 ) | hatoi(*(s++));
   return result;
}

You might want to check for overflow.

int hatoi(char c); /*** I'd suggest a longer, descriptive name
                        such as parse_hexdigit ***/

int main()
{
    char s[] = "0xfff"; /*** Is this a good test case?
                             It is a palindrome with no numbers 0-9 ***/
    …
}

int hatoi(char c)
{
  int res;
  if (isdigit(c))
    res = c - '0';
  else if (c >= 'a' && c <= 'f')
    /* offset a..f to 10-14 */ /*** 10 - 15 ***/
    res = (c - '0')-39; /*** res = c - 'a' + 10 is clearer ***/
  else if (c >= 'A' && c <= 'F')
    /* offset a..f to 10-14 */
    res = (c - '0')-7; /*** res = c - 'A' + 10 is clearer ***/
  else
    res = 0;
  return res;
}

int htoi(char s[])
{
  int len,i,result,power,digit;
  result = power = 0;

  len = strlen(s)-1; /*** Check for overflow when you can ***/

  for (i=len; i >= 0; --i) { /*** Avoid iterating backwards ***/
    digit = hatoi(s[i]);
    if (digit) {
      if (power == 0) {
        result += digit;
        power++;
      } else /*** Use a consistent pattern of braces ***/
        result += digit * power;
      power *= 16;
    }
  }
  return result;
}

I would write it like this:

unsigned htoi( char *s ) {
    unsigned acc = 0;

    if ( * s != '0' ) return 0;
    ++ s;
    if ( * s != 'x' || * s != 'X' ) return 0;
    ++ s;

    /* Check that multiplication by 16 will not overflow */
    while ( acc < UINT_MAX / 16 ) {
        if ( * s >= '0' && * s <= '9' ) {
            acc *= 16;
            acc += * s - '0';
        } else if ( * s >= 'A' && * s <= 'F' ) {
            acc *= 16;
            acc += * s - 'A' + 10;
        } else if ( * s >= 'a' && * s <= 'f' ) {
            acc *= 16;
            acc += * s - 'a' + 10;
        } else {
            return acc; /* handles end of string or just end of number */
        }

        ++ s;
    }

    return acc;
}

a simple version to handle positive hexdigits

int htoi(const char s[]) {
    unsigned int n = 0;
    int i = 0;
    if (s[0] == '0' && (s[1] == 'x' || s[1] == 'X')){
        i += 2;
    }
    int isValid = 1;  // check if hexdigit is valid
    for (; isValid; i++) {
        if (s[i] >= '0' && s[i] <= '9') {
            n = 16 * n + (s[i] - '0');
        }
        else if (s[i] >= 'a' && s[i] <= 'f') {
            n = 16 * n + (s[i] - 'a' + 10);
        }
        else if (s[i] >= 'A' && s[i] <= 'F') {
            n = 16 * n + (s[i] - 'A' + 10);
        }
        else {
            isValid = 0;
        }
    }
    return n;
}