I wrote a function to reverse a char-array (string). Since I'm beginner and didn't work with malloc and stuff before, maybe someone could take a look, if this is fine, what I'm doing here?

char* reverse_string(char* string)
{
    // getting actual length of the string
    size_t length = strlen(string);

    // allocate some space for the new string
    char* new_string = malloc(sizeof(char)*(length+1));

    // index for looping over the string
    int actual_index = 0;

    // iterating over the string until '\0'
    while(string[actual_index] != '\0') 
        new_string[length-actual_index-1] = string[actual_index++];

    // setting the last element of string-array to '\0' 
    new_string[length] = '\0';

    // free up the allocated memory
    free(new_string);

    // return the new string
    return new_string;
}
up vote 2 down vote accepted

Here are some things that may help you improve your code.

Fix the bug

Once memory is freed, it should not be referenced again. Unfortunately, your code allocates memory and then frees it and then returns a pointer to the freed memory. That's a serious bug! To fix it, simply omit the free within the function and make sure the caller calls free instead. Alternatively, you could avoid all of that by reversing the passed string in place.

Use the required #includes

The code uses strlen which means that it should #include <string.h> and malloc and free which means that it should #include <stdlib.h>. It was not difficult to infer, but it helps reviewers if the code is complete.

Use const where practical

In your revere_string routine, the string passed into the function is not and should not be altered. You should indicate that fact by declaring it like this:

char* reverse_string(const char* string)

Check for NULL pointers

The code must avoid dereferencing a NULL pointer if the call to malloc fails. The only indication that it has failed is if malloc returns NULL; if it does, it would probably make most sense to immediately return that NULL pointer.

Learn to use pointers instead of indexing

Using pointers effectively is an important C programming skill. This code could be made much simpler by doing an in-place reversal of the passed string and by using pointers:

char* reverse_string(char* string) {
    if (string == NULL) 
        return string;
    char *fwd = string;
    char *rev = &string[strlen(string)-1];

    while (rev > fwd) {
        char tmp = *rev;
        *rev-- = *fwd;
        *fwd++ = tmp;
    }
    return string;
}
  • thank you for your review. learned a lot of new things with it :) – Matthias Burger Feb 9 '17 at 21:24

Your Answer

 

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.