1
\$\begingroup\$

Is it possible to make this code more efficient & elegant?

int HexToDecimal(char ch)
{
    ch = tolower(ch);
    if ('a' <= ch && ch <= 'f')
    {
        return ch - 'a' + 10;
    }
    else if ('0' <= ch && ch <= '9')
    {
        return ch - '0';
    }
    printf("Error\n");
    return 0;
}

Update: Printing error could be ignored. By returning 0 I only mean to show that the input char is not a hex digit.

\$\endgroup\$
3
\$\begingroup\$

From memory (I haven't checked it) this should be the most efficient in terms of speed at the cost of a small increase in memory footprint.

Note that it does not repeat your printf("Error\n") smell.

return
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" // <nul> ... <si>
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" // <dle> ... <us>
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" // ' ' ... '/'
"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x00\x00\x00\x00\x00\x00" // '0' ... '?'
"\x00\x0a\x0b\x0c\x0d\x0e\x0f\x00\x00\x00\x00\x00\x00\x00\x00\x00" // '@' 'A' ... 'O'
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" // 'P' ... 'Z' ... '_'
"\x00\x0a\x0b\x0c\x0d\x0e\x0f\x00\x00\x00\x00\x00\x00\x00\x00\x00" // '`' 'a' ... 'o'
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" // 'p' ... 'z' ... '~' <del>
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" /// Accented/Extended
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
[c];

Added

Apologies for not being clear. This solution is not "better" than the original, just different and provably faster. I would use the originally posted code in all cases except when maximum speed is critical and only then after careful deliberation.

I posted this for completeness, it is not elegant in any way, it is only more efficient ... almost brutally so.

\$\endgroup\$
  • \$\begingroup\$ What about endianness? \$\endgroup\$ – Quentin Pradet Mar 15 '12 at 15:51
  • \$\begingroup\$ @Cygal: Not applicable because it is char. \$\endgroup\$ – Martin York Mar 15 '12 at 16:42
  • 1
    \$\begingroup\$ Clever but a -1 \0xFF to indicate error would have been useful so that you can check for error. \$\endgroup\$ – Martin York Mar 15 '12 at 16:44
  • \$\begingroup\$ I wish I knew what I was looking at. \$\endgroup\$ – Ryan Miller Mar 15 '12 at 17:30
  • 1
    \$\begingroup\$ @RyanMiller: Its a string with 256 characters. You the use the character c (which has a value of 0->255) as the index into the string. It returns a character from the string. The string is carefully constructed so that character 51 in the array returns the character \x03. Thus '3'(ie 51) when used as an index will return '\x03' which converts to the integer 3. \$\endgroup\$ – Martin York Mar 15 '12 at 19:26
2
\$\begingroup\$

The first idea comming to mind is to check 0 <= ch <=9 before tolower(ch), because if it's a digit, then you don't need to make tolower.

Say, what does it means if the function returns 0? How to determine the case "it returns 0 because out of hex digits range" and "because ch is 0"?

\$\endgroup\$
  • \$\begingroup\$ Updated the question. By returning 0 I only mean to show that the input char was not a hex digit. \$\endgroup\$ – Meysam Mar 15 '12 at 19:38
  • \$\begingroup\$ @Meysam, what does it return when you pass '0'? \$\endgroup\$ – Robert Gowland Mar 23 '12 at 16:28
1
\$\begingroup\$

As fogbit said, first check for digits, then tolower if necessary. By the way, unless you wrote your own tolower, it should probably be std::tolower.

Don't print anything on error. Either assert that the value is correct, return an impossible value (-1 is commonly used), or throw an exception. Printing that there was an error does not tell the rest of your program what happened.

Finally, do you really need to write this? Are you sure your numbers won't be in the form 0xABCD, which can be converted just fine with boost::lexical_cast or an std::stringstream wrapper?

\$\endgroup\$
1
\$\begingroup\$

Now for elegance (forgive me ... my C is a little rusty):

static char * Hex = "0123456789abcdef";
char * found = strchr(Hex, tolower(ch));
return found != null ? found - Hex: 0;

We search for the character in the string. If found then the value is the offset of the found character from the start of the string.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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