Hex to integer converter

Question

I've just coded a hex to integer converter in C and want my code criticized. I did not cover switch statements so I know that can be improved. I am also planning to truncate "0x" in the beginning of the input if the user enters that.

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

#define MAXCHAR 100

int htoi(char hexArray[], int srraySize);

int main(void){ 
    int userInput;
    int i = 0;
    char hexArray[MAXCHAR];

    while((userInput = getchar()) != EOF){
        if(userInput >= '0' && userInput <= '9'){
            hexArray[i] = userInput;
            ++i;
        }

        if((userInput >= 'a' && userInput <= 'f') || (userInput >= 'A' && userInput <= 'F')){
            hexArray[i] = userInput;
            ++i;
        }   
    }

    hexArray[++i] = '\0';

    int ans = htoi(hexArray, i);

    printf("%d\n", ans);

    return 0;
}

int htoi(char hexArray[], int arraySize)
    typedef enum{ a = 10,
        b,
        c,
        d,
        e,
        f
        } hexValues;

    int intArray[MAXCHAR] = { 0 };
    int i, j;
    int finalAns = 0;

    for(i = 0; i < arraySize; ++i){
        if(hexArray[i] >= 'A' && hexArray[i] <= 'F'){
            hexArray[i] = (char)tolower((int)hexArray[i]);
        }
    }

    for(i = 0; i < arraySize; ++i){
        if(hexArray[i] >= '0' && hexArray[i] <= '9'){
            intArray[i] = hexArray[i];

        } else if(hexArray[i] >= 'a' && hexArray[i] <= 'f'){
            if(hexArray[i] == 'a'){
                intArray[i] = a;
            } else if(hexArray[i] == 'b'){
                intArray[i] = b;
            } else if(hexArray[i] == 'c'){
                intArray[i] = c;
            } else if(hexArray[i] == 'd'){
                intArray[i] = d;
            } else if(hexArray[i] == 'e'){
                intArray[i] = e;
            } else if(hexArray[i] == 'f'){
                intArray[i] = f;
            }
        }
    } 

    j = arraySize-2;

    for(i = 0; i < arraySize-1; ++i){
        printf("J = %d\n", j);
        printf("Initial Value: %d\n", intArray[i]);
        intArray[i] = (intArray[i] * (pow(16, j)));
        printf("Processed Value: %d\n", intArray[i]);
        finalAns = finalAns + intArray[i];
        --j;
    }

    return finalAns;
}

Answer 1

• Your hexArray has a static size of MAXCHAR but you never check if you didn't exceed this size when reading input. Consider what would happen if someone enters MAXCHAR+1 characters at input.

• When you set last character in your hexArray to '\0', you do it on wrong position. In your while loop, you increment i after each write to hexArray so at the end i is an index of next free element. And you should write '\0' here. But you use pre-increment (++i) so you write this '\0' on next place.

• This '\0' is not really needed at all since you pass array size as an argument to htoi anyway. But you pass wrong size as I already mentioned.

• You are using tolower function in your htoi function but you could do this already when getting input. This saves you one loop over all values. It means that htoi will be less general since it will only work with low capital letters, though.

• You could also use tolower function on all the input characters since if tolower gets argument that is not uppercase letter, it will return unchanged value. This would save you some ifs in your code for the price of overhead of running a function (and doing ifs in it since it has to do this).

• if(hexArray[i] >= '0' && hexArray[i] <= '9') intArray[i] = hexArray[i]; is wrong. You set intArray[i] value to '0' (which is numerical value 48) instead of 0. So you should use intArray[i]=hexArray[i]-'0'; instead.

• Your big if-elseif block for a-f characters can be much simpler. All ASCII letters have consecutive numbers so to convert 'a' to 10, 'b' to 11 etc you can use:

    else if(hexArray[i] >= 'a' && hexArray[i] <= 'f') {
        intArray[i] = hexArray[i] - 'a' + 10;
    }
  

  This saves you a lot of ifs and you can even remove your enum.

• The whole intArray array is not really needed at all since you could do your computations inside of the loop in which you create it. This way you can have only one loop inside of your htoi.

• Yyou are using int type to hold computed integer. So you can really only convert sizeof(int)*2 characters long hex value inside, and your MAXCHAR=100.

• You can eliminate the need of pow function if you will start your calculations from the end of intArray array. You can store powers of 16 in some variable and just multiply it by 16 (or shift left 4 bits) in each iteration.