Word Inverter in C

Question

I've just made this program and I want to know how to improve it. It just receives a word and prints it backwards. Sequences of numbers are also accepted, but not white spaces (that's why I used scanf).

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

int main(void)
{
    const unsigned short MAX = 30;
    char array[MAX];
    unsigned short length;

    scanf("%s", array);

    length = strlen(array) - 1;
    char array2[length];

    for (unsigned short i = length, n = 0; i >= 0, n <= length; i--, n++)
    {
        array2[n] = array[i];
        printf("%c", array2[n]);
    }

    return 0;
}

Answer 1

Buffer overflow

char array[MAX];
scanf("%s", array);

This can overflow the buffer in array if the user input exceeds MAX-1 characters. There are multiple ways around this:

• Ask scanf to allocate an appropriately sized buffer (if you have a moderately recent glibc):

   char *array = NULL;
   scanf("%ms", &array);
  

  You need to release the allocated buffer later:

   free(array);
  

  To use free you need to #include <stdlib.h>.

• Use fgets() and inform it about the buffer size:

   fgets(array, sizeof(array), stdin);
  

  This will read a whole line though and not just a single word. It may also include the newline character at the end that you need to remove yourself:

   length = strlen(array) - 1;
   if (array[length] == '\n')
       array[length--] = '\0';
  

• getline() works in a similar fashion but also allocates a buffer for you; conveniently it also returns the amount of characters read (which may differ from the length of the allocated buffer stored in array_size):

   char *array = NULL;
   size_t array_size;
   size_t length = getline(&array, &array_size, stdin);
   // ... remove potential newline ...
   // ... do stuff with array ...
   free(array);
  

  getline requires glibc's GNU extensions (#define _GNU_SOURCE) or POSIX.1-2008 (#define _POSIX_C_SOURCE 200809L).

Use appropriate data types

• size_t for length since that's the return type of strlen and is guaranteed to hold all possible string lengths. You need to watch out for the corner cases below though to avoid an integer underflow (which you'd also have with any unsigned type).
• same for i since it depends on length.

Corner cases

• What if strlen(array) == 0? Then strlen(array) - 1 has a huge positive value since it underflows the unsigned return type of strlen and wraps around to the largest possible value of that type.

• In a similar fashion i >= 0 is always true since unsigned types cannot assume values less than 0.

You'll have to find a way to take care of these corner cases. What about simply doing nothing in that case since the reversion of the empty string is the empty string itself?

length = strlen(array);
if (length > 0) {
    for (size_t i = length - 1, n = 0; n < length; i--, n++) {
        // ...
    }
 }

I left out i >= 0 intentionally since it is redundant with n < length. It had also no effect since the , operator in C means that only the right side of it is responsible for the value of the entire (sub-)expression. You probably wanted && but, as I said, the two conditions are (nearly) equivalent here.

Unnecessary code and (possibly) better algorithms

You don't need an array2 if you end up printing the characters one by one. You can just print them directly from array:

printf("%c", array[i]);

Alternatively you can first revert the string and later print the whole string:

for (unsigned short i = length, n = 0; i >= 0, n <= length; i--, n++)
    array2[n] = array[i];

fwrite(array2, 1, length, stdout);

Or you can revert the string in-place without an additional buffer/array:

for (unsigned short i = length, n = 0; i > n; i--, n++)
{
    char c = array[n];
    array[n] = array[i];
    array[i] = c;
}

puts(array);

puts prints the whole string and appends a newline. By convention *nix programs always terminate their lines, including the last (unless they operate on raw bytes).

Better character output function

printf is very comfortable to use if you want formatted output but it's also pretty heavy-weighted. If you're only printing individual characters you can just use:

putchar(array[i]);

Error checking

You should check the return values of all functions that may fail if it impacts the outcome of the program. For instance:

• scanf, fgets, or getline may all return error conditions if an I/O error occurred that would mess up the remainder of the program. You should check for errors and react accordingly (i. e. shut down the program):

   if (scanf("%ms", &array) <= 0)
       // ...
  
   if (fgets(...) == NULL)
       // ...
  
   if (getline(...) == -1)
       // ...
  

  If an error occurs I suggest you print an error message and return an error condition yourself (which terminates the program if done from main()):

    if (...) {
        perror("Input error");
        return 1;
    }
  

  Terminating a program with a value other than 0 notifies the caller of an error by convention.

• In a similar fashion you can check for errors on printf, fwrite, puts, and putchar and react to them.

You can read about the error conditions of functions in their manual pages. On most *nices you can access those comfortably with man 3 <FUNCTION_NAME>.

Use expressive variable names

• array is a buffer for a string: string_buffer or shorter strbuf.
• i and n are the left and right indices of the reversion loop: left and right.

Final program

A possible final program could look like this:

#define _POSIX_C_SOURCE 200809L
#include <stdlib.h>
#include <stdio.h>

int main(void)
{
    char *strbuf = NULL;
    size_t strbuf_size;
    
    ssize_t length = getline(&strbuf, &strbuf_size, stdin);
    if (length == -1) {
        perror("Input error");
        return 1;
    }
    
    
    if (length != 0) {
      if (strbuf[length-1] == '\n')
            strbuf[--length] = '\0';
        
        if (length != 0) {
            for (size_t right = (size_t) length - 1, left = 0; left < right; right--, left++) {
                char c = strbuf[left];
                strbuf[left] = strbuf[right];
                strbuf[right] = c;
            }
        
            if (puts(strbuf) == EOF) {
                perror("Output error");
                return 1;
            }
        }
    }
    
    free(strbuf);

    return 0;
}

Answer 2

Pretty good, but I have a few notes:

• Nice job on putting void in for the parameter arguments for main()

• Extract the main algorithm into its own function, separate from main(). This helps compartmentalize the program into more maintainable parts if expanded in the future.

• A length of 30 can be short for a string, depending on what you use the program for. I'd recommend making that more variable by testing for the string length once you have an input string, and then creating a pointer based on that length. This makes the final implementation much more dynamic.

• Prefer uint16_t from stdint.h to unsigned short. I just prefer to use size_t, even though I doubt you'll go beyond the bounds of a short type. size_t is also the return type for strlen().

• You don't have to return 0 at the end of main(), just like you wouldn't bother putting return; at the end of a void-returning function. The C standard knows how frequently this is used, and lets you not bother.

  C99 & C11 §5.1.2.2(3)

  ...reaching the } that terminates the main() function returns a value of 0.

---

Final Program:

A quick note about something a few people will want to point out: a portion of the C standard defines the string handling function arguments, unless specified otherwise, must have valid values. I try to mimic that here, therefore excluding the NULL check. This also provides a slight boost in execution speed.

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


char* strrev(const char *str)
{
    size_t len = strlen(str);
    char *rev = malloc(len + 1);

    for(size_t i = 0; i < len; ++i)
    {
        rev[i] = str[(len-1) - i];
    }

    // need to NULL terminate the string
    rev[len] = '\0';
    return rev;
}

int main(void)
{
    // outputs "blimey nice function"
    puts(strrev("noitcnuf ecin yemilb"));
}

Answer 3

Your code won't compile in C89. If you want to increase the portability of your code and want it to be compilable in all C standards,

You should

1. Not use non-standard functions
2. Not use VLAs
3. Not declare variables in the middle of the code

Check this code:

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

#define xstr(s) str(s)
#define str(s) #s

#define MAX 30

int main(void)
{
    char array[MAX + 1];
    size_t length;

    if(scanf("%" xstr(MAX) "s", array) != 1)
    {
        fputs("scanf failed! Exiting...", stderr);
        exit(EXIT_FAILURE);
    }

    for (length = strlen(array); length-- ;)
    {
        putchar(array[length]);
    }

    return EXIT_SUCCESS;
}

Improvements:

• Compiles without any errors or warnings in C89, C99 and C11.
• Added error checking for scanf as well as a length modifier to prevent buffer overflows.
• Removed unneccessary variables.
• Reduced code complexity.
• Uses appropriate data types (size_t).
• Used putchar instead of printf which will result in better performance.

If you are wondering what those xstr and those weird #defines are, read up on Stringification.

Answer 4

This is a basic program of string reverse. Instead of defining a new array, which will again take some amount of memory, you can do in-place swapping in the array.

The code will look something like this:

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

    int main(void)
    {
        const unsigned short MAX = 30;
        char array[MAX];
        unsigned short length, i, j;

        scanf("%s", array);
        length = strlen(array) - 1;

        for (i = 0, j = length; i < j; i++, j--)
        {
            char temp = array[i];
            array[i] = array[j];
            array[j] = temp;
        } 

        printf("%s\n", array);

        return 0;
    }

Answer 5

You should simplify the following loop to make it more readable:

    for (unsigned short i = length, n = 0; i >= 0, n <= length/2; i--, n++)
    {
        array2[n] = array[i];
        printf("%c", array2[n]);
    }

There is no need to push it all in the for clause:

    unsigned short i = length;
    for (unsigned short n = 0; n <= length; n++)
    {
        array2[n] = array[i];
        printf("%c", array2[n]);
        i--;
    }

The following needs fewer variables and is even more clear:

    for (unsigned short n = 0; n <= length; n++)
    {
        array2[n] = array[length-n];
        printf("%c", array2[n]);
    }

You do not need another array to invert your word; you only need an additional variable:

    for (unsigned short n = 0; n <= length/2; n++)
    {
        temp = array[n];
        array[n] = array[length-n];
        array[length-n] = temp;
        printf("%c", temp);
    }