1

I'm trying to reverse a string in C with following function:

void reverse(char *txt) {
  char *copytxt;
  copytxt = (char*) malloc((strlen(txt) + 1 )  * sizeof(char));
  strcpy(copytxt, txt);

  int i;
  for(i=0;i<strlen(copytxt);i++){
    if(i == strlen(copytxt)){
      *(txt+i) = 0;
    }
    else{
      *(txt+i) = *(copytxt+strlen(copytxt)-i-1);
    }
  }
}

When i print *(txt+i)as a char in each loop of the for-loop. I'll get my reversed string. But if i print the string txtit just gives me nothing. Why is that? What am I doing wrong? Am I doing something wrong with the pointers?

By the way: I'm not allowed to use this notation: txt[1]

I hope you get my problem.

Thank you!

Bernd Strehl
  • 2,852
  • 4
  • 24
  • 43
  • Better try to do it inplace, your algorithm should be `O(n)`. Also, you got a memory leak, don't forget to `free`! Next, [Don't cast the result of malloc (and friends)](http://stackoverflow.com/questions/605845). I recommend reading [How to Debug Small Programs](http://ericlippert.com/2014/03/05/how-to-debug-small-programs) to get a better idea of how to debug this. Something more: `sizeof(char)` is per definitionem 1. – Deduplicator Apr 29 '14 at 14:31
  • 1
    Show your `printf` statement. That is where the error lies. – Carey Gregory Apr 29 '14 at 14:32
  • 1
    Are you passing a `character array` or a `character pointer` to `reverse()`? – alvits Apr 29 '14 at 14:50

4 Answers4

1

UPDATED:

You have a least memory leak in your code. When you reverse a string result should be returned somehow (e.g. make function not void but char*):

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

char* reverse(char *txt) 
{
  char *copytxt = (char*) malloc((strlen(txt) + 1)  * sizeof(char));
  strcpy(copytxt, txt);

  int i;
  for(i=0;i<strlen(txt);i++)
  {
    if(i == strlen(txt)){
      *(txt+i) = 0;
    }
    else
    {
      *(copytxt+i) = *(txt+strlen(txt)-i-1);
    }
  }

  return copytxt;
}

The function work correctly (probably the mistake in you main function), below is the my case:

int main()
{
    char txt[] = "Test me";
    char* copytxt = reverse(txt);
    printf("%s\n", copytxt);
    return 0;
}
mblw
  • 1,762
  • 1
  • 19
  • 28
  • But if i print ``*(txt)`` I get a ``g`` for the string ``"reverse my string";`` I thought I addressed this problem with the ``-1``in the 4th last row – Bernd Strehl Apr 29 '14 at 14:26
  • 2
    This answer is incorrect. In fact, the code works fine for me. – Carey Gregory Apr 29 '14 at 14:30
  • @Strernd You get 'g' because `*(txt)` refers to the single char at position `txt[0]`. It is **not** a string. – Carey Gregory Apr 29 '14 at 14:31
  • I have segfault with "Test me" string, debugging – mblw Apr 29 '14 at 14:32
  • Returning a char would be my way to do it, too. But our professor told us that it is a good practice to change the var in place and to return nothing (or an code for success or failure). In fact most of us are new to C and are slightly overstrained by the complexity of the code we are given. – Bernd Strehl Apr 29 '14 at 14:57
  • @Strernd Yes in C-language it's a commonly used practice to pass all data by mean of function arguments and return value used to indicate routine status (if error occurred), but your has bad error. Yes, you edit your string in place but you create a copy of original string and not `free` it. So in your code `free(copytxt)` required before exit from function otherwise if your program works for a long time as daemon (for example) computer memory will be continuously fill with `copytxt` buffer (that's named memory leak). – mblw Apr 29 '14 at 15:39
1

Are you sure this:

for(i=0;i<strlen(copytxt);i++){
    if(i == strlen(copytxt)){

is fine?

This:

i == strlen(copytxt)

Should never hold, due to this:

i=0; i < strlen(copytxt);i++

I suppose? (due to this: <, i never gets value of strlen(copytxt))

1

You can actually do a string reversal in-place, without needing to allocate more memory:

void swap(char *lhs, char *rhs) {
    char temp = *lhs;
    *lhs = *rhs;
    *rhs = temp;
}

void reverse(char *txt) {
    int length = strlen(txt);

    int i;
    for (i=0; i<length/2; ++i) 
        swap(txt + i, txt + length - i - 1);
}
Bill Lynch
  • 80,138
  • 16
  • 128
  • 173
0

The if block bellow will never get executed

if(i == strlen(copytxt)){
  *(txt+i) = 0;
}

Better put 

*(txt+i) = 0;

after the for loop

Ari
  • 92
  • 5