reverse string in c using pointers

Question

I am trying to reverse the string using pointers but it seems that it does not work, what's the problem? the output is olllo but should be olleh.

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


void reverse(char *cadena){

    size_t len = strlen(cadena);
    char *end= cadena;
    char *aux = cadena;
    while(*++end){}
    --end; //end points to a

    for(;len;--len){
        *aux++ = *end--;

    }

}

int main()
{

    char buffer[] = "hello";
    reverse(buffer);
    printf("%s",buffer);


    return 0;
}

Your for-loop is broken; you should swap each character pair and run only up to the half of the string. — Ctx
– Ctx, Commented Jun 24, 2018 at 22:38
@SHG is correct, you can simply calculate end using the length directly. char *end= cadena + (len - 1); — Ilan Keshet
– Ilan Keshet, Commented Jun 24, 2018 at 22:43

Fureeish · Accepted Answer · 2018-06-25 10:57:34Z

3

The line:

*aux++ = *end--;

Does not swap anything. It just assigns the left side the value of the right side. You'll always end up with a palindrome, made from the right half of the string. For swap logic, you should do:

char tmp = *aux;
*aux = *end;
*end = tmp;

Also, you shouldn't really iterate through the whole string. Actually, it'd mean reversing the string and then reversing it back again. Just apply the swapping logic while iterating through just half of the string, and you're good to go:

void reverse(char *cadena) {
    if(cadena != NULL){                                 // add some checks...    
        size_t len = strlen(cadena);
        char *end = cadena + (len > 1 ? len - 1 : 0);   // ...for safety    
        char *aux = cadena;

        for (len /= 2; len; --len) {
            char tmp = *aux;
            *aux++ = *end;
            *end-- = tmp;
        }
    }
}

edited Jun 25, 2018 at 10:57

answered Jun 24, 2018 at 22:50

Fureeish

13.5k4 gold badges39 silver badges70 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

Deduplicator Over a year ago

Well, I don't know why you decided to define NULL as nop, I would generally prefer a fast abort to make debugging easier. But it's not actually wrong. I would also try to reduce branching...

Fureeish Over a year ago

Im sorry but this isn't code review. Here the main goal was to demonstrate the idea, not to provide the optimal solution

gon1332 · Accepted Answer · 2018-06-24 23:33:09Z

1

Addressing your fault

The problem with your loop is that while you try to reverse the characters in place you end up swap already swapped characters.

I'll try to explain by showing what happens in each iteration. You can see the initial contents of cadena and the final (res) after each swap in each iteration. The | are where the pointers aux and end are currently pointing:

len = 5
aux  |  
     h e l l o
end          |
res: o e l l o

len = 4
aux    |  
     h e l l o
end        |
res: o l l l o

len = 3
aux      |  
     h e l l o
end      |
res: o l l l o

len = 2
aux        |  
     h e l l o
end    |
res: o l l l o

len = 1
aux          |  
     h e l l o
end  |
res: o l l l o

len = 0
=> break the loop

On a solution..

My inplace reverse would be this one:

void reverse(char *str)
{
    if (!str || !(*str)) return;

    char *end = str + strlen(str) - 1;

    while (str < end) {
        char tmp = *str;
        *str++ = *end;
        *end-- = tmp;
    }
}

Some key points:

notice the use of strlen to find the last character of the string
you should iterate until the starting and ending pointers meet. Otherwise you re-reverse the string

edited Jun 24, 2018 at 23:33

answered Jun 24, 2018 at 22:58

gon1332

2,1102 gold badges27 silver badges30 bronze badges

2 Comments

Deduplicator Over a year ago

You also suffer from UB when handed the empty string. Fix that by reconsidering where to decrement.

gon1332 Over a year ago

@Deduplicator Thanks for the remark. Had it in mind but never written it.

Deduplicator · Accepted Answer · 2018-06-24 23:44:55Z

There are three important errors in your code:

You are mishandling the empty string when advancing to the end in this snippet:
```
char *end= cadena;
// Irrelevant: char *aux = cadena;
while(*++end){}
--end; //end points to a
```
Anyway, you already have the string-length, so just add it.

When fixing this error, make sure you only create valid pointers, meaning to or just behind objects.
You are going over the length of the whole string, instead of just half of it. If you actually swapped the elements, you would thus swap everything twice, a quite expensive nop:
```
for(;len;--len){
```
You are copying instead of swapping. You didn't save the value you overwrote, and you didn't store it back where it belongs.
```
    *aux++ = *end--;
}
```

Fixed code:

void reverse_string(char* s) {
    char* e = s + strlen(s);
    while (s < e) {
        char t = *s;
        *s++ = *--e;
        *e = t;
    }
}

Collectives™ on Stack Overflow

reverse string in c using pointers

3 Answers 3

2 Comments

2 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

2 Comments

2 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related