patterncMinor
Converting decimal integers to a string representation in an arbitrary base between 2 and 26
Viewed 0 times
arbitrarydecimalbetweenrepresentationandconvertingintegersstringbase
Problem
This code takes an integer and returns a string representing the value in a different base. The value for the base can range between 2 and 26. I have had someone already look over this code and they told me that I was committing a "cardinal sin" of memory management and referred me to the documentation for
I have also used this for testing:
```
int main(){
char * output = calloc(100, sizeof(char));
int ret_val = 0;
//test cases
ret_val = change_base(output, 100, 37, 8);
printf("%s\t%d\n", output, ret_val);
ret_val = change_base(output, 100, -45, 8);
printf("%s\t%d\n", output, ret_val);
ret_val = change_base(output, 100, 33945877, 8);
printf(
malloc(). I am confused as to what he is referring to. The only thing I have come up with so far is that I do not check to see if memory allocation succeeds when using calloc() or malloc(). Are there any other things that could or should be improved?char int_to_char(int number){
if (number > 9) return (char)(((int)'A') + number - 10);
else return (char)(((int)'0') + number);
}
int change_base(char* output, int buffer_size, int decimal_number, int base){
//check for valid parameters
if((base 26)) return -1; //range error
//ready variables
int output_i = 0;
int tmp_string_i = 0;
int dividend;
char remainder;
char * tmp_string = calloc(buffer_size, sizeof(char));
memset(output, '\0', buffer_size*sizeof(char));
//check for negative input
if(decimal_number buffer_size){ //+1 for the extra negative sign
free(tmp_string);
return -2; //buffer size error
}
}
//add last digit to string
remainder = int_to_char(dividend);
tmp_string[tmp_string_i] = remainder;
//copy tmp_string to output in reverse order
for(; tmp_string_i >= 0; tmp_string_i--){
output[output_i] = tmp_string[tmp_string_i];
output_i++;
}
free(tmp_string);
return 0;
}I have also used this for testing:
```
int main(){
char * output = calloc(100, sizeof(char));
int ret_val = 0;
//test cases
ret_val = change_base(output, 100, 37, 8);
printf("%s\t%d\n", output, ret_val);
ret_val = change_base(output, 100, -45, 8);
printf("%s\t%d\n", output, ret_val);
ret_val = change_base(output, 100, 33945877, 8);
printf(
Solution
-
The cardinal sin
An assignment
-
Unnecessary variables
Reversal can be done in-place. There is no need for
-
Finding digits
Executing division
-
No raw loops
Most loops represent an important algorithm, which deserves to be factored out in a function of its own, for further reuse and for giving it a name. In your case, the loop under the
All that said, this is
-
Misc
-
The returned string should be terminated by '\0', or its actual length be returned.
-
The code happily converts numbers to a base up to 36 (10 digits and 26 letters). I see no reason to limit a base to 26.
-
Return codes
-
Size variables (e.g.
-
The
The cardinal sin
An assignment
tmp_string[tmp_string_i] = remainder; may write beyond an allocated memory. Consider a case of buffer_size being equal to 0. You must test tmp_string_i before the assignment. I don't know what exactly someone had in mind, but you are definitely committing one.-
Unnecessary variables
decimal_number is passed by value; you can mutilate it as much as you want. There is no need to have dividend.Reversal can be done in-place. There is no need for
tmp_string at all. tmp_string_i shall also be gone.-
Finding digits
Executing division
dividend / base twice per iteration looks wrong; special-case of the last digit looks also wrong. Using do-while solves both wrongs:do {
output[output_i++] = int_to_char(dividend % base);
number = number / base;
} while (number != 0);-
No raw loops
Most loops represent an important algorithm, which deserves to be factored out in a function of its own, for further reuse and for giving it a name. In your case, the loop under the
//copy tmp_string to output in reverse order comment implements an algorithm known as reverse.All that said, this is
-
Misc
-
The returned string should be terminated by '\0', or its actual length be returned.
-
The code happily converts numbers to a base up to 36 (10 digits and 26 letters). I see no reason to limit a base to 26.
-
Return codes
0,-1,-2 should be defined as named constants.-
Size variables (e.g.
buffer_size) should be unsigned, or better yet of type size_t.-
The
decimal_number parameter is not decimal. It is just number. Need to be renamed accordingly.Code Snippets
do {
output[output_i++] = int_to_char(dividend % base);
number = number / base;
} while (number != 0);Context
StackExchange Code Review Q#67360, answer score: 3
Revisions (0)
No revisions yet.