5
\$\begingroup\$

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 < 2) || (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 < 0){
        output[0] = '-';
        dividend = -decimal_number;
        output_i++;
    }
    else dividend = decimal_number;

    //find digits
    while(dividend / base != 0){
        remainder = int_to_char(dividend % base);
        dividend = dividend / base;
        tmp_string[tmp_string_i] = remainder;
        tmp_string_i++;
        if(tmp_string_i + 1 > 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("%s\t%d\n", output, ret_val);
    ret_val = change_base(output, 100, 32, -7);
    printf("%s\t%d\n", output, ret_val);
    ret_val = change_base(output, 100, 37, 4);
    printf("%s\t%d\n", output, ret_val);
    ret_val = change_base(output, 100, 37, 23);
    printf("%s\t%d\n", output, ret_val);
    ret_val = change_base(output, 100, 37, 2);
    printf("%s\t%d\n", output, ret_val);
    ret_val = change_base(output, 100, -375212123, 26);
    printf("%s\t%d\n", output, ret_val);
    ret_val = change_base(output, 5, -375212123, 26);
    printf("%s\t%d\n", output, ret_val);
    char * output2 = calloc(5, sizeof(char));
    ret_val = change_base(output2, 5, 37, 2);
    printf("%s\t%d\n", output2, ret_val);

    free(output);
    free(output2);
    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ dividend = -decimal_number; certainly has trouble when decimal_number == INT_MIN; \$\endgroup\$ – chux - Reinstate Monica Nov 1 '14 at 3:14
3
\$\begingroup\$
  • 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.

\$\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.