A function that reverses a string of characters

Question

My problem is in writing a function reverse(s) that reverses the character string s, a line at a time. My code here works, I write the line and it reverses the line. Is it a good solution?

#include <stdio.h>
#include <stdlib.h>
#define MAX 1000

void  reverse(char cad[] ,char cadenita[],int i);

int main() {
    int a ,i ;
    char cad[MAX] = {0,0};
    char cadenita[MAX];
    i= 0;
      while ((a = getchar()) != EOF) {
          if (a != '\n') {
             cad[i] = a;
              ++i;
          }
          else  {                  
              cad[i] = '\0' ;              
             reverse(cad , cadenita, i);             
              printf("%s\n",cadenita);           
              i = 0;
          }                  
   }     
    return (EXIT_SUCCESS);
}

void  reverse(char s[] ,char svol[], int i) {
    int a =0 ;
    // i is the amount characters in s

    while ((svol[a] = s[i-1]) != '\0' ){
        ++a;
        --i;       
    }   

} 

Also in this part of code while ((svol[a] = s[i-1]) != '\0' ){ . If I change to ((svol[a] = s[i-1]) != '\n' ) it still works and gives the same result, but why? I think there is an issue. Can you explain? Also, where does it break?

Answer 1

A smarter use of getchar()

I don't recommend using getchar() to read one character at a time, since you are really interested more in lines. However, if you do use getchar(), you might as well use it more effectively, by writing the string in reverse to begin with.

#include <stdio.h>

#define MAX 1000

int main() {
    char cadena[MAX];
    do {
        int i;
        cadena[i = MAX - 1] = '\0'; 
        while (i > 0) {
            int c = getchar();
            if (c == EOF) {
                return 0;
            } else if (c == '\n') {
                break;
            } else {
                cadena[--i] = c;
            }
        }
        puts(cadena + i);
    } while (1);
}

Answer 2

Naming

The mix of Spanish and English is disconcerting: cad vs. s. The use of cadenita for the reversed string also seems odd, since the reversed string is the same length as the original. svol is even weirder, as s appears to be short for "string" (English) and vol, I'm guessing, is short for "volvió" (Spanish). Keep in mind that the function's name, reverse, is English. (Personally, I recommend using English everywhere.)

i is conventionally used as an iteration dummy variable rather than as a length parameter. If you mean length, then call the parameter len or length. Using size_t instead of int would also be a good hint to its purpose.

Function declaration

Does reverse() operate on NUL-terminated strings or on a byte array of a specified length? You should make up your mind, since bugs tend to occur when the two conventions get mixed up. If the function takes a length, then it shouldn't look for '\0'. Alternatively, if looks for '\0', then it shouldn't accept a length parameter.

Since allocating memory is an annoyance in C, I would consider it idiomatic for the reverse() function to operate on the string in place rather than making a copy. If the caller wants to keep the original string intact, then make it the caller's responsibility to duplicate the string first.

It is conventional to place main() last, so that you don't need to make forward declarations of the functions that it calls.

Input

I don't recommend reading a character at a time using getchar(). Character-at-a-time I/O is less efficient. More importantly, using a more appropriate function to read an entire line can simplify your code. Using getline(), for example, makes your intentions clearer, and it avoids the possibility of a buffer overflow.

#include <stdio.h>

char *reverse(char *s, size_t len) {
    for (int i = 0, j = len - 1; i < j; i++, j--) {
        char swap = s[i];
        s[i] = s[j];
        s[j] = swap;
    }
    return s;
}

int main() {
    char *line = NULL;
    size_t linecap = 0;
    ssize_t linelen;
    while ((linelen = getline(&line, &linecap, stdin)) > 0) {
        if (line[linelen - 1] == '\n') {
            line[--linelen] = '\0';
        }
        puts(reverse(line, linelen));
    }
}

Answer 3

Bug

I don't think your reverse function is correct. If you step through the loop, you can see that it will dereference past the start of s (into s[-1], s[-2], etc). You are probably just getting lucky that s[-1] happens to be 0. I think your function should look like this:

void reverse(const char *s, char *svol, int len)
{
    int i = 0;
    // len is the amount characters in s

    while (len > 0)
        svol[i++] = s[--len];
    svol[i] = '\0';
}

Notice I added a const qualifier to the argument that doesn't change. I also renamed your variables because i is usually a loop index not a length argument.

Answer 4

I have a few problems with how you are being taught, but that's off-topic so I'll just comment on your code.

• Standard C does not have a string type, so it's a bit of an error in terminology for you to talk about string (that's in C++) and String (that's in Java and C#).
• The closest thing to a string in C is a char * -- but that is not a string. It's an array (looks like you studied that already) of char. There are certain library functions that can perform useful work on a char *, but they still don't make it a string.
• You really need to learn pointers in order to understand what's going on with char *. I suggest asking in programmers.se
• You're not making use of the library functions you do have. If you want to read a whole line at a time, use fgets(str,num,stream) (check the documentation). For completeness purposes there's also gets(str) but you shouldn't ever use it, it's unsafe.
• And, instead of printf("%s",str) use fputs(str,stdout).
• It's confusing that the work of your while loop is in the condition, and the body of the loop only incrementing the indices.