Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm learning C and one of the questions I've been asked is converting a string to an integer. The code I've written supports converting from a string in any base up to 16/hex to an integer. There is no over/underflow checking and it does not support lowercase hex.

int ASCIIToInteger(char *x, int base)
{
//Each element is the ASCII for it's index. 
int ASCIICodes[] = { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F' };  _Bool negative = 0;
int count = 0, output = 0;  
if (x[0] == '-')
{
    negative = 1;
    count = 1;
}
else if (x[0] == '+')
{
    count = 1;
}
do
{
    for (int i = 0; i < base; i++)
    {               
        if (ASCIICodes[i] == (int)x[count])
        {
            output = output * base + i;
            break;
        }

    }
    count++;
} while (x[count] != 0);

if (negative == 1)
{
    return ~output + 1;
}
return output;

}

Can I please get some advice regarding any possible issues or "rookie mistakes" I may have made?

share|improve this question
    
Is this intended to be reinventing the strtol (2) function? – Mast 1 hour ago
up vote 7 down vote accepted

Avoid mysterious bit-shifting

return ~output + 1;

Is the same as:

return - (output + 1) + 1;

And as:

return - output;

But the last one is way more obvious than the first one.

Do not hide variable declarations

int ASCIICodes[] = { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F' };  _Bool negative = 0;

If I look at the above line, the least thing in my mind is that a variable is defined after that list.

If a user has a small screen or a big font size he will be very puzzled when he will see you use negative without seeing its definition.

Just use a newline, they are free:

int ASCIICodes[] = { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F' };
_Bool negative = 0;

Make variables as local as possible

I used x++ to express that I will skip the first character:

if (x[0] == '-')
{
    negative = true;
    x++;
}
else if (x[0] == '+')
{
    x++;
}

This allows me to make count local.

Using for loops when practical

The outer of your loops is a do while, but looking around I found all three components of a for loop, just shuffled into the code.

A for loop will give it more organization:

for (int count = 0; x[count] != 0; count++)
{
    for (int i = 0; i < base; i++)
    {               
        if (ASCIICodes[i] == x[count])
        {
            output = output * base + i;
            break;
        }
    }
}

Simplify the initial conditional

Assigning a boolean to negative directly and using a || conditional looks shorter and simpler:

We go from:

_Bool negative = 0;
int count = 0, output = 0;  
if (x[0] == '-')
{
    negative = 1;
    count = 1;
}
else if (x[0] == '+')
{
    count = 1;
}

to:

bool negative = x[0] == '-';

if (x[0] == '-' || x[0] == '+')
{
    x++;
}

While preserving identical functionality.

it's vs its

... How to choose between it's and its?

This is actually really easy, do you mean "it is" or not?

Use it's when you mean it is.

// Each element is the ASCII for it's index. 

Here you should write its because you mean possession, intended as a property of the element.

Saying that:

// Each element is the ASCII for it is index.

Makes no sense.

So the correct comment will be:

// Each element is the ASCII for its index. 
share|improve this answer

Use bool

Instead of _Bool, use bool, for example:

#include <stdbool.h>

int ASCIIToInteger(char *x, int base)
{
    // ....
    bool negative = false;
    // ...
    if (x[0] == '-')
    {
        negative = true;
        count = 1;
    }

    // ...

    if (negative)
    {
        return ~output + 1;
    }

One statement per line

Avoid multiple statements on the same line, for example instead of:

int ASCIICodes[] = { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F' }; _Bool negative = 0;

Break that up to multiple lines:

int ASCIICodes[] = { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F' };
_Bool negative = 0;

This is because it's easier to read from top to bottom. If there are statements at the end of lines, that can be very distracting.

Pointless cast

No need to cast to int here:

if (ASCIICodes[i] == (int)x[count])

Simpler ASCIICodes

A simpler way to define ASCIICodes:

char ASCIICodes[] = "0123456789ABCDEF";

Ternary operator

Instead of this:

if (negative)
{
    return ~output + 1;
}
return output;

You could simplify using the ternary operator:

return negative ? -output : output;

Or perhaps instead of a bool negative, you could use an int sign:

int sign = 1;
int count = 0, output = 0;  
if (x[0] == '-')
{
    sign = -1;
    count = 1;
}
// ....
return sign * output;
share|improve this answer

You don't check for non-number characters. The sign is in the while-loop silently ignored, as any other symbol. What is the number of "HELLO" in hex? 14.

share|improve this answer

Accept both cases by folding input through toupper().

#include <ctype.h>
//...

    if (ASCIICodes[i] == toupper((int)x[count]))

But you can also define the set as a string and use strchr to do the searching.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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