Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I am currently studying c programming, but I'm at a very basic level. So for practice I made a program that reads a string and inverts it. I want to know if the code is readable and how I can improve it.

/* 
* Goal: Given a string, invert it, then print the result.
* Author: Lúcio Cardoso
*/

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

/*This is the size of the arrays in this program. It's better to use a          
constant, because makes easier to modify the arrays' size, if needed.*/
#define NUM 200


void invert(char *s) //Fucntion responsible for inverting the string.
{
    char aux[NUM];
    int i, j, n; //These variables are all used to control the for loops.

    /*This loop will read the size of the array's string, that's why
    strlen(s) - 1 is used. We don't need to read all contents
    from the array, just the string. As we read the s[] backwards,
    its content is stored in aux[n]. In this case, from zero to the
    size of s[]'s string - 1. Minus 1 is used because we read it from ZERO,
    not i. */
    for (i = strlen(s) - 1, n = 0; n < strlen(s), i >= 0; i--, n++) {
        aux[n] = s[i];
    }

    printf("Included with the super mojo from the string inverter, ");
    printf("this is the result: ");

    //This for loop reads all the content from the aux array, and print it.
    for (j = 0; j < strlen(s); j++) {
        printf("%c", aux[j]);
    }
}

int main(void)
{
    char string[NUM];

    printf("\nWelcome to the super, duper string inverter!\n");
    printf("If you want to see some magic, enter a string:\n>");
    gets(string); //Reads the string.

    //Now, we only need to call the function with string[] as a parameter.
    invert(string);

    return 0;
}
share|improve this question
    
I'm not sure about C but I know some languages (VB, C#, Swift) have string methods that allow you to invert your string. – SyntaxBit 3 hours ago
up vote 5 down vote accepted

That's a fine small piece of software for a beginner. Here is my review:

const-modifier
Since you don't modify the contents of the char *s you can safely change your function signature to void invert(const char *s).

Too many strlen() calls
You execute strlen(s) every for-loop iteration. This is quite bad for the performance (especially for large strings) since strlen loops through your whole string each call. If your string is 100 characters long (beside the NULL-character) this ends up in 100*100 = 10000 iterations.
As a quick solution you could just create a variable length and store the length of your string once. From that point on you'll compare with length instead of strlen(x) and will get the same result (since, s doesn't change while execution).

Comparison unused n < strlen(s) remains unused in your first for-loop. I think you want to connect the two comparisons with &&:

for (i = strlen(s) - 1, n = 0; n < strlen(s) && i >= 0; i--, n++)

Why even reverse?
Since your function prints the reversed string, but doesn't return anything. You don't really need to reverse it. You could just loop through the input string starting at the end (like you did in your first loop), printing all the characters. The extra array only makes sense, if you return a pointer to it for later use.

share|improve this answer
    
Thanks for the tips! – Lúcio Cardoso yesterday
3  
There are ways. You should look into memory-management. This is a quiet important topic for C-programmers, but you'll get to it in a little while. My best tip: Read K&R: The C-Programming Language - 2nd Edition. It's out there as a PDF for free and contains a whole lot of exercises and explains the most important features of C very well. Keep up the good stuff. – LastSecondsToLive yesterday

Usability

Your function isn't very flexible. If another piece of code needs to use it, it will have to use it for a single purpose, and that purpose is to be given a string and to print out that string in reverse order.

Below are some ways you could make this more flexible:


User-supplied output buffer

Right now, the reverse string is just printed straight to STDOUT. To make this more flexible and so that it follows the single responsibility principle, you could have the function accept an output buffer to stick the reversed string into:

void reverse(char *s, char *out) {

With this, you should also remove the printf calls in your function.

Note: an alternative to this would be to overwrite the original string. Rather than accepting an output buffer, you can create a temporary output buffer in the function (like you are with aux). Then, simply copy the temporary output buffer into the input string at the end of the function. However, I am unsure if this is good practice or not (also, it removes some versatility from the function).


User-supplied length

What if the code using this function doesn't want to have the entire string reversed? What if they only want the first part of the string reversed? To allow for this, you function should accept the amount of characters to reverse:

void reverse(char *s, char *out, size_t len) {

Then, you simply use that len in your loops as you have been already.


Your code

Enough of that; time to review the code you have presented.


Printing out a string

for (j = 0; j < strlen(s); j++) {
    printf("%c", aux[j]);
}

Here, you are looping through the aux "array" and printing out each character. However, you are over-complicating it; why not just print aux?

printf("%s", aux);
share|improve this answer
    
Thank a lot buddy! – Lúcio Cardoso 23 hours ago
  • You're using printf to print a single char:

    printf("%c", aux[j]);
    

    printf is a very handy tool to format output, but it carries quite a bit of overhead with it – not something you want to call over and over without a good reason. A much more efficient way to print a single char to stdout would be:

    putchar(aux[j]);
    

    Or, instead of looping over the string to print it character-by-character, you can printf the whole thing with just one function call:

    puts(aux);
    

    (Note that puts prints an additional newline character at the end of the string. If you want to avoid that, use fputs(aux, stdout) instead.)

    puts and fputs require the string (aux) to be terminated by a NUL-character ('\0'), which it isn't in your program. If you want to print a character sequence of known length (but possibly without such a terminator character), you can use:

    fwrite(aux, 1, strlen(n), stdout);
    
  • You can also use puts or fputs to print all the string literals in your program with less overhead, e. g.

    puts("\nWelcome to the super, duper string inverter!");
    

    instead of

    printf("\nWelcome to the super, duper string inverter!\n");
    

    Now you don't even need to worry about % characters being interpreted as format descriptors.

  • You can concatenate string literals that are printed right after each other to avoid the overhead of additional buffering and I/O locking and unlocking:

    fputs("Included with the super mojo from the string inverter, this is the result: ", stdout);
    

    If you don't like long string literals you can break them into multiple parts:

    fputs("Included with the super mojo from the string inverter, "
          "this is the result: ", stdout);
    

    produces exactly the same syntax tree and binary code as the source code just before.

share|improve this answer
    
If using puts(), be sure to NUL-terminate the string first. – 200_success 19 hours ago
    
@200_success: True, but the break condition of the loop in question is based on a comparison with the result of strlen() anyway. – David Foerster 19 hours ago
1  
Though s is NUL-terminated, it's not certain that aux is NUL-terminated. – 200_success 19 hours ago
    
Good catch! I'll edit my answer. – David Foerster 19 hours ago
    
Thanks a lot David! Your tips will surely be useful! – Lúcio Cardoso 17 hours ago

Never, ever, use gets(). It is flawed by design: it does not limit the length of the input it accepts. A buffer overflow is possible if the input is too long, and gets() will let it happen. The function exists only for compatibility with old code.

Rather, you should use fgets(string, length, stdin) — in your case, fgets(string, NUM, stdin).

share|improve this answer
    
Thanks a lot bro! Well, I would have used fgets, the only reason I didn't, is that I haven't learned it yet. Now, I'll certainly use it! – Lúcio Cardoso 23 hours ago

You've done a lot of work to comment and clarify your code, but I think it's actually too much commenting. Sometimes that reads as a wall of text instead of helpful brief comments. For instance:

/*This is the size of the arrays in this program. It's better to use a          
constant, because makes easier to modify the arrays' size, if needed.*/
#define NUM 200

The first sentence is very verbose, while the rest is just a programming principle. If that's for your own reference as a beginner that's fine, but most programmers will know that. I'd suggest having no comment, and renaming your constant ARRAY_SIZE, as that you what you need to know anyway.

Comments are good, but most effective when short as they're more likely to be read that way. Usually people will know what the common functions of a program do, but comments can help illuminate what you're doing or why you need to do things a certain way. For your first for loop in invert, I'd keep it to a simple comment:

// Loop backwards over s and store it in aux
share|improve this answer
    
Thanks a lot! Also, forgive for my English mistakes, it's just that i'm Brazilian, so, you know. – Lúcio Cardoso yesterday
    
@LúcioCardoso Glad to help! Your english was fine. If you mean the edit I made to your question, that's just a bit of tidying that people often do on the site. It's nothing to worry about! – SuperBiasedMan yesterday

You need to ensure that your aux string is null terminated. Add the line aux[strlen(s)] = '\0'; right after the first for loop.

share|improve this answer
1  
Thanks for the tip! – Lúcio Cardoso 17 hours ago
1  
Welcome to Code Review! Good job on your first answer. – SirPython 17 hours ago
    
If you null terminate it, then you can use printf("%s"... instead of using another for loop. – Jesse Weigert 14 hours ago

Remember that strings are null terminated char arrays. Like this:

char my_string[] = {'a','b','c','\0'}; 

I would have called NUM, MAX_STRING (or something like it). aux, should then be

char aux[MAX_STRING +1] //+1 for null terimination

The name aux, is also not good. Use descriptive names on variables, like string_inverted.

Also your function is invert a string and print it, not invert a string. Its better to let a function only do one thing, this let you use your functions as primitives for building other functions. The prototype could be like this.

void invert(const char *string,char * inverted_string);

Also if you use a function only inside one compilation unit (c file), make them static.

As other have pointed out, you comment a lot. An ideal code (that don't exist) should not need comments. Use comments to aide code reading when code deviate from this ideal. If you use comments for documenting code, which can be a good idea, specially if you work with others, look in to doxygen.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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