2
\$\begingroup\$

Is there any way I can make this code more efficient? I'm new to C, any suggestion is welcome!

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

#define LIMIT 1024

unsigned long htoi(char s[], int len);

int main(void){
    char line[LIMIT+1];

    int i=0;
    for(int c=0; i<LIMIT-1 && (c = getchar()) != '\n' && c != EOF; ++i){
        line[i] = c;
    }
    line[i] = '\0';

    printf("%lu\n", htoi(line, i));
}

unsigned long htoi(char s[], int len){

    unsigned long total = 0;
    unsigned short currdigit = 0;
    int power = 0;

    for(int i=len; i>-1; --i){

        if(s[i] == 'X' || s[i] == 'x' || s[i] == 0){
            continue;
        }

        if(s[i] >= '0' && s[i] <= '9'){ // if it is a numerical char
            currdigit = s[i]-'0';
        }
        else{
            if(s[i] >= 'a' && s[i] <= 'z'){ // if it's a lower case char
                currdigit = s[i] - 'a';
            }
            else{ // if not, then we assume it is an upper case char (should I?)
                currdigit = s[i] - 'A';
            }

            currdigit+=10;
        }

        total += (currdigit * pow(16, power));

        ++power;
    }

    return total;
}

I have a few questions as well:

  1. The book I am using to learn C didn't reach the point of teaching pointers yet; however, I do know at least the basics. Is there any way pointers could be used in this program?
  2. Should I make change the parameter char s[] to const char s[] since I am not changing any of its elements' values? Is that common practice?
  3. Should I use short instead of int when I know the number won't surpass great quantities? As an instance, when developing a game, if I know my player's health is from 0 to 100, should I be using a short to store that value? And should it be unsigned as well if I know it won't be negative?
\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

#define LIMIT 1024: magic constants should be named well. What is the limit for? You are also storing the result in an unsigned long and contain much less than 1021 hex digits (the last 3 are for 0x and the null terminator).

Operators should be spaced for better readability: int i = 0; and for (int

I'm a bit rusty with C, but I think fgets can be used instead of rolling your own getc loop

Inside htoi, a single tolower or toupper removes the need to check lowercase and capitals

if(s[i] >= 'a' && s[i] <= 'z'): hex digits only contain 0-9 and a-f (not a-z)

pow(16, power): in this case, pow(16, power) = 1 << (4 * power) (see bitshifts). I would split the 4 * power so that it is more clear what it represents (digit position)

Some changes to the algorithm are necessary: input of 1z00x000 gives 53477376 and kibe gives 86718. I would check if there is a leading 0x and if there is, to skip it (if not, return an error code)

if not, then we assume it is an upper case char and And should it be unsigned as well if I know it won't be negative: with your code, I can get currdigit to underflow by choosing (for example) a plus sign as input.

\$\endgroup\$
5
  • \$\begingroup\$ gets() should not be used. Lookup "c gets function dangerous", e.g. stackoverflow.com/q/1694036/1187415. \$\endgroup\$
    – Martin R
    Commented Apr 19, 2020 at 6:31
  • \$\begingroup\$ 1 << (4 * power) is a problem with forming an unsigned long. Recommend 1UL << (4 * power). \$\endgroup\$ Commented Apr 21, 2020 at 18:15
  • 1
    \$\begingroup\$ thank you so much for your help! i'm still learning bitwise operators and I hope I'll be proficient enough to use them in my code; is it always best to use bitwise operators when you can? (as in, performance-wise). \$\endgroup\$
    – kibe
    Commented Apr 21, 2020 at 20:28
  • \$\begingroup\$ @kibe Bitwise operators should be used when they make the code's intention clear. The performance difference of code using bitwise operators and non-bitwise operators on modern compilers and computers barely make a difference (with very few exceptions). I would suggest reading discussions on performance vs readability 1, 2. \$\endgroup\$ Commented Jun 12, 2020 at 6:43
  • \$\begingroup\$ @kibe Please consider accepting an answer \$\endgroup\$ Commented Jun 12, 2020 at 6:45
1
\$\begingroup\$

In addition to @FromTheStackAndBack good answer:


Is there any way I can make this code more efficient?

Wrong tool for the job, bug

pow(16, power) can be a fairly time expensive to call a floating point function for an integer problem. Weak implementations may not provide the exact value sought, perhaps just under the expected integer and lead to the wrong answer with integer truncation. Best to stay with integer math.

currdigit * pow(16, power) will lead to loss of precision when long is 64 bit and double with only 53. --> Wrong result.

Rather than range tests, research is...() functions.

Simplify

Suggest building the value from s[0] to the end.

// Algorithm
sum = 0;
while (not done) {
  sum = sum*16 + char_to_digit(char)
  next char
}

Is there any way pointers could be used in this program?

Yes - see below. Note that s of char s[] is a pointer.

Should I make change the parameter char s[] to const char s[] since I am not changing any of its elements' values? Is that common practice?

Yes, you should - it is common. It also allows code to call with a const char[].

Should I use short instead of int when I know the number won't surpass great quantities?

No. int is the size the processor is typically best at both in speed and code footprint. Use short when you have an array or many instances of that type.

If pressed, could use int16fast_t and perhaps reap some benefit. Yet this is a micro-optimization for 1 or 2 variables.

As an instance, when developing a game, if I know my player's health is from 0 to 100, should I be using a short to store that value?

Yes, or even unsigned char if you have lots of players. Else no. For one player - do not concern about this. Code for clarity.

should it be unsigned as well if I know it won't be negative?

IMO yes, but you will find various opinions on this one.


Other observations

Leading X

if(s[i] == 'X' || s[i] == 'x'...) allows for an X anywhere in the char[]. Usually an X is only allowed as a leading character or as 0x.

No detection of non-hex characters

No detection of overflow

No allowance for a sign character

String

unsigned long htoi(char s[], int len) operation on a char array without needing a terminating null character as in a string. This may be the goal, yet more C-like to expect a final null character and drop the need for len.


Sample alternative. TBD: how to handle errors?

#include <ctype.h>
#include <limits.h>
#include <stdbool.h>

unsigned long htoi(const char *s) {
  unsigned long value = 0;
  bool digit_found = false;

  // Allow leading spaces?  This is common
  while (isspace((unsigned char ) *s)) {
    s++;
  }

  // Perhaps allow for a sign?
  int sign = *s;
  if (*s == '-' || *s == '+') {
    s++;
  }

  // Detect X, 0X prefix - adjust to needs
  if (*s == 'X' || *s == 'x') {
    s++; // consume it
  } else if (*s == '0' && (s[1] == 'X' || s[1] == 'x') && 
      isxdigit((unsigned char) *s[2])) {
    s += 2;
  }

  while (isxdigit((unsigned char ) *s)) {
    int digit;
    if (isdigit((unsigned char) *s)) digit = *s - '0';
    else digit = toupper((unsigned char) *s) - 'A' + 10;

    if (value >= ULONG_MAX / 16) {
      // we have overflow - various ways to handle. Here code just limits
      value = ULONG_MAX;
    } else {
      value = value * 16 + (unsigned) digit;
    }
    digit_found = true;
    s++;
  }

  if (!digit_found) {
    // Error, nothing to convert, ignore for now
  }

  // Allow trailing spaces?  This is not common, but I like it.
  while (isspace((unsigned char ) *s)) {
    s++;
  }

  if (*s) {
    // Error, trailing non-numeric text, ignore for now
  }

  if (sign == '-') {
    // TBD is this OK, sure -0 is OK, but should -123 wrap or what?
    // For now, just make 0
    value = 0;
  }

  return value;
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ kibe, Minor: with char line[LIMIT+1]; ... i<LIMIT-1, use i < LIMIT or i < sizeof line - 1. \$\endgroup\$ Commented Apr 21, 2020 at 18:32
  • 1
    \$\begingroup\$ chux, thank you so much for taking your time to better my code, seriously! I understood every bit of your example; however, why is it needed to cast unsigned char every time you check if it is a digit? (and when calling toupper as well). either way, thank you so much! \$\endgroup\$
    – kibe
    Commented Apr 21, 2020 at 20:25
  • \$\begingroup\$ @kibe Only a concern when x < 0. is...(int x) functions are defined for int values in the unsigned char range and EOF. When char ch is signed and ch < 0, is...(ch) is UB. When processing characters, code should use values in the 0-255 range. Same for to...(). \$\endgroup\$ Commented Apr 21, 2020 at 20:49

Your Answer

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

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