Reverse a word in the char array

Question

I have to write a program that reads a word to the char array and next display it reversed. I must find best own optimal solution.

My solution of this exercise:

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

char* reverse(char* str);

int main(void) {
    char word[50];
    printf("Podaj slowo: ");
    scanf("%s", word);
    printf("Odwrocone slowo: %s\n", reverse(word));
    return 0;
}

char* reverse(char *str) {
    char* reversed = (char*) malloc(sizeof(char) * strlen(str) + 1);
    int j = 0;
    for (int i = strlen(str) - 1; i >= 0; i--, j++) {
        reversed[j] = str[i];
    }
    reversed[j + 1] = '\0';
    return reversed;
}

I'm applying linear method, specifically iterating through every elements and store it in a new pointer array but I'm not sure if there is any better solution that I don't know. Can anyone add hints how can I optimize code and review it?

Answer 1

Memory management

Right now, your code leaks memory--every time you call reverse, it allocates some memory, and none of your other code frees it again.

If all you're doing is reversing one string, then exiting, that's of little consequence--but if you try to use this in real code, leaking memory like this is generally unacceptable.

const correctness

Since you're not modifying the input string, you might as well use const in the function's signature:

char* reverse(char const * str);

Buffer overrun protection

Right now you have:

scanf("%s", word);

This is essentially equivalent to gets(word);. That is to say, it provides absolutely no protection against the user entering a string longer than you've provided space to store. When using %s with scanf (or cousins like fscanf, sscanf, etc.) you need to specify the maximum length:

scanf("%49s", word);

Alternatively, consider using fgets, which also requires you to specify the buffer size.

Note that there's a difference in the size you specify though. With fgets, you specify the size of the buffer, but with scanf you specify the number of characters it's allowed to read, which is one less than the size of the buffer itself. Also note that fgets normally retains the \n at the end of what was entered though (if you get data without a \n on the end, it means the buffer you supplied wasn't large enough to hold all the data that was entered).

Unnecessary Cast

When you allocate your memory:

char* reversed = (char*) malloc(sizeof(char) * strlen(str) + 1);

...you're currently casting the result of malloc. This is generally frowned upon by C programmers--it's unnecessary and it can cover up a bug of having failed to include the right header to declare/prototype malloc correctly.

char* reversed = malloc(sizeof(char) * strlen(str) + 1);

The cast is only necessary if you decide to write C that can also compile as C++. This tends to give the worst of both worlds, and should generally be avoided--if you're going to compile this as C++, use std::string and std::reverse.

Size Computation

When you allocate memory, you multiply the length by sizeof(char), but sizeof(char) is guaranteed to be 1, so the multiplication accomplishes nothing.

Even if you decide to leave the multiplication in (since it is necessary for types other than char), I prefer to use code like this:

char* reversed = malloc(sizeof(*reversed) * strlen(str) + 1);

This has the advantage that when/if you (for example) decide to support wide characters, you can change it to something like:

wchar_t* reversed = malloc(sizeof(*reversed) * (wcslen(str) + 1));

...so the argument to sizeof doesn't need to change at all (and believe me--if you decide to do something like this, you have enough headaches to deal with, so even a small help will be welcome).

Answer 2

Bug: off-by 1

// reversed[j + 1] = '\0';
reversed[j] = '\0';

Avoid mixing int/size_t types.

size_t and int have 1) different sign-ness and 2) potential far different positives ranges with size_t usually more than int. size_t is the right-size type for array indexing and string lengths.

"how can I optimize code" --> No need to call strlen() twice. Weak compilers will iterate the length of str on each call when only once is needed.

Check malloc() results

// Use size_t index for i, j 
char* reverse_alloc(const char *str) {
  size_t i = strlen(str);
  char* reversed = malloc(sizeof *reversed * (i + 1));

  if (reversed) {
    size_t j = 0;
    while (i > 0) {
      i--;
      reversed[j] = str[i]; 
      j++;
    }
    reversed[j] = '\0';
  }

  return reversed;
}

To reverse in place:

char* reverse_in_place(char *str) {
  size_t len = strlen(str);
  size_t i = 0;
  while (len > i) {
    char tmp = str[--len];
    str[len] = str[i];
    str[i++] = tmp;
  }
  return str;
}