1

I'm making a function which reverses a string and checks if the string is a palindrome or not. When I test the function with a obvious palindrome like "abba", the function says it's not a palindrome. Also the forward string and the reversed string also differ in string length!

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

char forward [] = "abba"; //The string to be reversed

int size = (sizeof(forward)/sizeof(forward[0]) - 1);
int j = 0;
char reverse [10];

void reverser(char forward []) {

    printf("%s", "String forward: ");
    for (int i = 0; i < size; i++) { //Function for printing out the forward string.
        printf("%c", forward[i]);
    }

    printf("\n");

    printf("%s", "String reversed: ");

    for (int i = size, j = 0; i >= 0; --i, ++j) { //Function for reversing the string and printing it.
        reverse[j] = forward[i];
        printf("%c", reverse[j]);       
    }

    printf("\n");

    if (strcmp(forward, reverse) != 0) { //Using strcmp to check if the forward and reversed string are the same. 
        printf("Not a palindrome!\n");
    }
    else{
        printf("A palindrome!\n");
    }

    printf("Forward string length: %d\n",strlen(forward));
    printf("Reversed string length: %d\n",strlen(reverse));
} 

int main () {
    reverser(forward);      
}

Output:
String forward: abba
String reversed: abba
Not a palindrome!
Forward string length: 9
Reversed string length: 0

xgord
  • 4,606
  • 6
  • 30
  • 51
Wickerman
  • 367
  • 2
  • 4
  • 8

4 Answers4

1

In this loop

for (int i = size, j = 0; i >= 0; --i, ++j) { //Function for reversing the string and printing it.
    reverse[j] = forward[i];
    printf("%c", reverse[j]);       
    }

forward[i] is the terminating zero of the string when the initial value of variable i is equal to size

Try the following loop implementation

for ( int i = size, j = 0; i != 0; ++j) { //Function for reversing the string and printing it.
    reverse[j] = forward[--i];
    printf("%c", reverse[j]);       
    }

Also take into account that for variables that will store values returned by the operator sizeof or by the function strlen it is better to use type size_t.

So in these statements

printf("Forward string length: %d\n",strlen(forward));
printf("Reversed string length: %d\n",strlen(reverse));

use format specifier %zu instead of %d

For example

printf("Forward string length: %zu\n",strlen(forward));
printf("Reversed string length: %zu\n",strlen(reverse));

Some answerers here said that you have to zero-terminate the string reverse. However it is already zero-terminated because it is declared outside any function and as result has the static storage duration and as such it is zero-initialized by default.

Vlad from Moscow
  • 301,070
  • 26
  • 186
  • 335
  • 1
    Also note that `reverse` is not null terminated. – chqrlie Dec 04 '15 at 16:56
  • @chqrlie Why do you have decided so? – Vlad from Moscow Dec 04 '15 at 16:57
  • Because even if it is not strictly necessary thanks to `reverse` being a global variable and the operation happening only once, I believe the OP is not aware that his code cannot be used in a more general manner. – chqrlie Dec 04 '15 at 17:07
  • @chqrlie You said that I should note that reverse is not null terminated.However it is a wrong statement.:) – Vlad from Moscow Dec 04 '15 at 17:13
  • OK. let's put it this way: it would be good advice for the OP remind him to null terminate `reverse` so the function works in the general case. Obviously the use of global variable `size` is not good practice either, the arbitrary size of 10 bytes is questionable too... – chqrlie Dec 04 '15 at 17:53
0

I think the first char of "reversed" is '\0'.

Replace this line int size = (sizeof(forward)/sizeof(forward[0]) - 1); by int size = strlen(forward).

Moreover :

printf("%s", "String forward: "); for (int i = 0; i < size; i++) { //Function for printing out the forward string. printf("%c", forward[i]); }

can be replaced by printf("String forward : %s\n", forward);

William B.
  • 41
  • 4
0

You are putting the null character in the first index when reversing (reverse[0]). That's why every fails.

for (int i = size, j = 0; i >= 0; --i, ++j) {         
    reverse[j] = forward[i];
}

should be

for (int i = size - 1, j = 0; i >= 0; --i, ++j) {
    reverse[j] = forward[i];
}
reverse[size] = '\0'; 

sizeof(forward)/sizeof(forward[0]) is 5 which means size = 4.

forward[0] = 'a'
forward[1] = 'b'
forward[2] = 'b'
forward[3] = 'a'
forward[4] = '\0'
UmNyobe
  • 22,539
  • 9
  • 61
  • 90
0

The reversing code is off by one on one of the indices, i should run from size-1 to 0 inclusive:

// Function for reversing the string.
for (int i = size, j = 0; i > 0;) {
    reverse[j++] = forward[--i];
}
reverse[size] = '\0';

To avoid updating 2 different indices, you can simplify this loop:

// Function for reversing the string.
for (int i = 0; i < size; i++) {
    reverse[i] = forward[size - 1 - i];
}
reverse[size] = '\0';

There are many other issues in your code:

  • forward and size are global variables, but you also name the argument in function reverser the same way and use the global size there, which may be inappropriate if called with a different argument. size and even worse j should not be global variables (!)

  • function name reverser is inconsistent with what the function does: printing its argument.

  • you can print the argument string directly with a %s printf format. No need for a loop.

  • %d is not the correct format for strlen() return value. Either cast as (int)strlen(forward) or use %zu.

chqrlie
  • 131,814
  • 10
  • 121
  • 189
  • You should make your code easier to understand by removing the `j++` and `--i` from those brackets and place them in their proper place in the for-loop – smac89 Dec 04 '15 at 16:48
  • @Smac89: I put them there on purpose actually. Downward loops should not use `>= 0` because this test is inappropriate for unsigned variables. Pre-decrementing the index upon use or postdecrementing it in the test expression is idiomatic, albeit confusing for beginners. The proposed alternative is simpler and less error prone. – chqrlie Dec 04 '15 at 16:53