current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 3 down vote favorite
1

Problem: Write a function to convert Integer to String, using only pointers, without array indexes, string must be dynamically allocated.

#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>

// move each character in array one place to the right
// we need to make place for new character on the left
void moveArrayElementsRight(char *ptr, int len) {
    for (int j = len; j > 1; j--) {
        *(ptr + j - 1) = *(ptr + j - 2);
    }
}

void intToStr(int myNumber, char* myString){
    int i = 1;    //track size of allocated memory
    bool isMinus = false;

    if (myNumber < 0) {

        myNumber *= -1;    //without this (myNumber % 10) + '0' wont work
        isMinus = true;
    }

    if (myNumber == 0){    //special case for 0

        myString = (char*)realloc(myString, ((i + 1) * sizeof(char)));
        *myString = '0';
        *(myString + 1) = '\0';
    }

    while (myNumber != 0) {

        myString = (char*)realloc(myString, ((i + 1) * sizeof(char)));  //increse memory =+1 for next digit
        i++;
        moveArrayElementsRight(myString, i);
        *myString = (myNumber % 10) + '0'; //put digit to array
        myNumber /= 10;
    }

    if (isMinus) {

        myString = (char*)realloc(myString, ((i + 1) * sizeof(char)));  //increse memory =+1 for '-' sign
        i++;
        moveArrayElementsRight(myString, i);
        *myString = '-'; //put sign at the beginning
    }
}

int main() {

    int numberToConvert = -10;
    char *numberAsString = (char*)malloc(sizeof(char));  //create empty array, with place only for '\0'
    *numberAsString = '\0';    //mark the end of array

    intToStr(numberToConvert, numberAsString);

    printf("%s", numberAsString);

    return 0;
}
share|improve this question
2  
There is no need to cast to (char*) when you malloc() or realloc(). It works perfectly fine without the casts. You should make it a habit to free() allocated memory on the heap to avoid memory leaks. – Phrancis Jan 5 at 7:44
1  
@Phrancis That sounds like the beginning of a helpful answer. – Graipher Jan 5 at 10:02
    
There's two things I would change, 1) Pre-allocate sufficient space to eliminate the need to realloc, and 2) Just place digits and optionally the minus sign on the right and reverse the string at the end. – Jonathan Watmough Jan 6 at 16:58
up vote 3 down vote

Issues Concerning Allocation

It is not necessary to cast the result of a call to malloc() in C, and many consider this to be a more error-prone style. It is easier to write correctly, read, and maintain the allocation line if you write it like this:

char *numberAsString = malloc(sizeof(*numberAsString));

This way, if the type is changed at some point in the future, there is only one place you need to update, versus three places to update in your version.

I would also suggest moving the allocation from main() into the intToStr() function (which is already reallocating the memory), and returning a pointer to the allocated memory to the calling function. This would necessitate changing the function signature of intToStr() to:

char * intToStr(int myNumber, char* myString);

Then in main(), you should declare:

char *numberAsString = NULL;

The same advice given above for malloc() applies to the calls to realloc() in the function intToStr(). Additionally, realloc() returns a NULL pointer in the event of an error. By assigning the return value directly to myString you risk leaking memory by losing the pointer to the previous allocation. You should store the return value of realloc() in a temporary variable, and then assign it to myString if it is non-null.

Continuing with allocation issues, I would consider mallocing enough memory to hold any int number-string (really, I would suggest using an int32_t number-- see below) at the beginning of intToStr(). Then, before returning the pointer to this memory, you can realloc() to the size of the actual number-string, i.e.:

number_string_sz = 12;  // for int32_t plus space for `-` and `\0`

myString = malloc(number_string_sz);

...

temp = realloc(myString, strlen(myString) + 1);
if (temp) {
    myString = temp;
}

You may wish to return a NULL pointer from intToStr() if there is an allocation error, and let the caller handle the error. Of course, the caller is responsible for freeing allocated memory.

Integer Overflow

Note that it is possible that myNumber is equal to INT_MIN, and then the line myNumber *= -1; would lead to integer overflow (invoking undefined behavior for signed integer types). Consider using a fixed-width type, such as int32_t, that is passed into the function, and copy this to a larger fixed-width type (int64_t) for a working copy within the function. The reasoning here is that the widths of the integer types are implementation-dependent, and even long int is only guaranteed to be at least as wide as int. By choosing specific widths for your integer types, you can protect against integer overflow in your calculations. The int64_t type is larger than int32_t, so the smallest value which can be held by an int32_t, when stored in an int64_t, can safely be multiplied by -1.

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.