patterncMajor
Converting a string to an Integer in C
Viewed 0 times
convertingstringinteger
Problem
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.
Can I please get some advice regarding any possible issues or "rookie mistakes" I may have made?
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?
Solution
Avoid mysterious bit-shifting
Is the same as:
And as:
But the last one is way more obvious than the first one.
Do not hide variable declarations
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
Just use a newline, they are free:
Make variables as local as possible
I used
This allows me to make
Using
The outer of your loops is a
A
Simplify the initial conditional
Assigning a boolean to
We go from:
to:
While preserving identical functionality.
... How to choose between it's and its?
This is actually really easy, do you mean "it is" or not?
Use
Here you should write
Saying that:
Makes no sense.
So the correct comment will be:
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 practicalThe 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.Code Snippets
return ~output + 1;return - (output + 1) + 1;return - output;int ASCIICodes[] = { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F' }; _Bool negative = 0;int ASCIICodes[] = { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F' };
_Bool negative = 0;Context
StackExchange Code Review Q#115749, answer score: 25
Revisions (0)
No revisions yet.