21
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ "char array2[length];" which you've declared in the middle of the code, using a value that isn't known until run time? Suggest you declare it at the top, with size MAX. (If you don't want the overhead of letting scanf allocate a buffer - I'm assuming you would want the reversal to run on multiple words in input?) \$\endgroup\$ – jamesqf Apr 24 '16 at 21:53
  • \$\begingroup\$ Actually, not multiple words. Only one. \$\endgroup\$ – Lúcio Cardoso Apr 24 '16 at 22:18
  • \$\begingroup\$ But it's usually a good idea to write reusable code. Say (since you say you're a beginner) this is assignment #1 in your programming class. What are the odds of assignment #2 being to reverse every word in a file? If you've written your reverse code as a function, you're almost done. \$\endgroup\$ – jamesqf Apr 25 '16 at 5:35
  • 1
    \$\begingroup\$ for (unsigned short i = length, n = 0; i >= 0, n <= length; i--, n++) this is not C. it's C++. change it to: unsigned short i; for (i = length, n = 0; i >= 0, n <= length; i--, n++) \$\endgroup\$ – Nick Apr 25 '16 at 11:05
  • 1
    \$\begingroup\$ @Nick You are wrong. That is C. The differences you stated are between C89 and C99. \$\endgroup\$ – syb0rg Apr 25 '16 at 23:25
30
\$\begingroup\$

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, and putc 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 <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;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ You can also use scanf() with a fixed-size buffer, but limiting the string length. \$\endgroup\$ – 200_success Apr 24 '16 at 19:24
  • \$\begingroup\$ @200_success: I looked at that too and it seems a bit unwieldy for a beginner. Too much pre-processor hackery… It's also limited to INT_MAX characters I believe. \$\endgroup\$ – David Foerster Apr 24 '16 at 19:29
  • 1
    \$\begingroup\$ putc requires two arguments not one. And I don't think it is a standard function. Perhaps you were thinking of putchar? \$\endgroup\$ – Spikatrix Apr 25 '16 at 5:11
  • \$\begingroup\$ @CoolGuy: You're right. I meant putchar. FYI they're both part of C89 and C99. \$\endgroup\$ – David Foerster Apr 25 '16 at 17:17
10
\$\begingroup\$

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"));
}
\$\endgroup\$
  • \$\begingroup\$ I just have a question: Why is it necessary to null-terminate the string? \$\endgroup\$ – Lúcio Cardoso Apr 24 '16 at 19:32
  • 2
    \$\begingroup\$ @LúcioCardoso It can lead to some nasty bugs if you don't, undefined behavior is going to occur. For example, if I pass a string into strlen() without a terminating character, it will keep searching memory until it a) finds said terminating character (which it won't); or b) hits an address that causes a memory protection fault of some sort. Imagine what would happen if you passed in that same string into strcpy(). It'll cause all sorts of death and destruction! \$\endgroup\$ – syb0rg Apr 24 '16 at 19:40
  • 2
    \$\begingroup\$ @LúcioCardoso: Many string handling functions in C assume that strings are terminated by null characters. If none exists at the end of the intended string it will work on the “garbage” in the memory behind the string until it encounters a null character (or provokes a memory access violation). This may provide invalid results and potentially reveal or overwrite important other values located in the memory next to the string buffer. Of course, some functions (or variants thereof) allow you to specify the string length through a parameter instead of a null-character at the end of the buffer. \$\endgroup\$ – David Foerster Apr 24 '16 at 19:44
  • \$\begingroup\$ I suggest renaming your function. There is a library function (non-standard) called strrev which is availiable in some implementations. This might cause nasty errors or even silent Undefined Behavior as this guy had when he used a function named strdup. \$\endgroup\$ – Spikatrix Apr 25 '16 at 5:05
  • \$\begingroup\$ Also, check malloc's return value and free the malloced memory \$\endgroup\$ – Spikatrix Apr 25 '16 at 5:06
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ It would have been nice if I didn't get a warning message cor C99. \$\endgroup\$ – pacmaninbw Apr 25 '16 at 11:22
  • \$\begingroup\$ Sorry, did not get you. What warning? \$\endgroup\$ – Spikatrix Apr 25 '16 at 11:32
  • \$\begingroup\$ Sorry, I was not referring to your answer, I was commenting that when I did compile the original question yesterday I got a warning message on the for loop. \$\endgroup\$ – pacmaninbw Apr 25 '16 at 11:35
2
\$\begingroup\$

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;
    }
\$\endgroup\$
2
\$\begingroup\$

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);
    }
\$\endgroup\$
  • \$\begingroup\$ Your loop in the last example will not work, because it swaps every pair of characters twice, which will simply output the original string. (The printed text will be reversed, but the value stored in the string won't.) You should have the loop go to length/2 so that each swap only happens once. (With 2 arrays, you do need the full length in the loop, but not with 1.) \$\endgroup\$ – Darrel Hoffman Apr 25 '16 at 14:16
  • \$\begingroup\$ @DarrelHoffman: You aree right, I changed the loop. Thank you. \$\endgroup\$ – miracle173 Apr 27 '16 at 16:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.