HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Converting decimal integers to a string representation in an arbitrary base between 2 and 26

Submitted by: @import:stackexchange-codereview··
0
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 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 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.