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. – RhysO Jan 23 at 16:37
    
@SyntaxBit There is no such function in the standard C library. – 200_success Jan 24 at 4:36
    
NEVER use gets() as it will allows the user to overrun the input buffer. It has been depreciated for a while and completely removed in the latest C standard. Suggest using fgets( string, sizeof(string), stdin ); – user3629249 Jan 25 at 2:03
    
when #define'ing numeric values, always surround the value with parens to avoid any 'text replacement' errors – user3629249 Jan 25 at 2:05
    
note: strlen() actually returns a size_t value, not a int value. To avoid the implicit conversions, use size_t` – user3629249 Jan 25 at 2:06
up vote 9 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
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 Jan 22 at 18:26

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: a better 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. As 200_success states,

I do recommend treating s as an in-out parameter, and reversing the string in place. If the caller wants to keep a copy of the original, then they can duplicate it themselves first. Placing the responsibility of buffer allocation on the caller reduces memory-management headaches in C.

Basically, by having the output go to the input string, memory management becomes much less complex and is overall easier on the caller.


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
2  
I do recommend treating s as an in-out parameter, and reversing the string in place. If the caller wants to keep a copy of the original, then they can duplicate it themselves first. Placing the responsibility of buffer allocation on the caller reduces memory-management headaches in C. – 200_success Jan 24 at 4:35
    
@200_success That makes sense. I've referenced you in my post and updated it to have a bit more information on it (however, I did not remove the original part for reference (or, should I just remove it?)). Thanks! – SirPython Jan 24 at 18:52
  • 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 print 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 Jan 23 at 0:23
    
@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 Jan 23 at 0:31

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 Jan 22 at 21:18

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
    
If you null terminate it, then you can use printf("%s"... instead of using another for loop. – Jesse Weigert Jan 23 at 5:31

There are a number of ways to approach this problem, but I like doing it in one of two (maybe archaic but simple) ways. You did well using just arrays but I'll explain a good way to approach this using beginners C level functions.

One way to invert a string is to run a for loop (or while loop) just like you did. However, you should familiarize yourself with pointers so you don't have to run your second for loop to print out all of the characters individually. You're already implementing some pointer logic with your arrays, but I'll add some information that should be helpful.

Character Pointers and Strings

Say I want a string that holds 100 characters, but I want to make use of pointers instead of starting with an array. Simply create a char* and assign it a 100 slots of memory with malloc() (+1 slot for the null character which terminates the string). malloc stands for memory allocate.

char* string;
string = (char*)malloc(101*sizeof(char));
//char string[101] is the same as this code above

Here string holds 101 places in memory that are size char (8 bits). String can be used just like an array to look at any one character. If we now store a bunch of information in this area with fgets() or scanf(), we can access any one character just like you did with aux[j].

for (j = 0; j < strlen(s); j++) {

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

}

Printing an Entire String

However, if we want to print the entire string after assigning the values/characters, simply use %s instead of looping through the entire array for each character. Let's use my predefined string pointer.

printf("%s", string);

Here you need to make sure your string ends with a null character '\0' or this printf will print out unassigned space in memory converted to character form.

Inverting a String

Do as you just did, by assigning the last position of your initial string (s) to the first position of your inverted string (aux) and continue on throughout the array. You did this well.

Freeing Pointer Memory

Finally, don't forget to free the memory you used and don't need anymore when moving on in your programs. For this simple project, freeing the memory you allocated for the pointer shouldn't matter all that much but is good for you to practice. This works only for things you've used malloc() with. This will free up the space you reserved (once you free, you're not getting that data back).

free(string);

Simple pointer code:

char* string;
string = (char*)malloc(101*sizeof(char));
//creates a pointer and gives it 101 slots of memory size character

fgets(string,100,stdin);
//retrieves input of 100 through stdin (your input) and stores in string

printf("You entered: %s\n", string);
//prints out what you stored in string

printf("A single character: %c\n", string[5]);
//prints out what you stored in position 5 of string
//assuming your string made it to at least 5

free(string);
//clears out the memory locations you reserved for string

Further Pointer Information

Pointers are confusing for most to learn about at first. They just point to information, they don't know what it is or care about what happens to it. Think of a pointer as a guy whose job is to point at your wallet so you always know where it's at. The guy doesn't care about how much money is inside, he just tells you the location of the wallet whenever you ask him. De-referencing his information (going to the location yourself and looking at the value) will tell you how much money is inside your wallet.

You can have two pointers pointing to the same thing, or one pointer pointing at another pointer. A pointer can direct you toward 1 thing or have it point at many things by using malloc(). You can reuse the same char* after freeing it, if you plan to point to more than one thing you need to malloc() however many spaces. A pointer can be an int, char, float, etc. all predefined with a *.

int* a;
float* b;
char* c;
dog* d; //after you "typedef dog"
char**e; //used for creating a list of pointers or 2D arrays
int f[300][300]; //same as char** e but of type int;
//etc

//if you want to use a string for later use after freeing it, point to NULL
c = NULL;

When I was learning to better implement pointers, I watched a video by suckerpinch about adding words to one another and making a Portmanteau. If you want to better familiarize yourself with pointers, try making a simple version of this where you concatenate strings (adding strings to one another - possibly helpful function: strcat()), print strings out frequently, etc. If you can make something like this, you'll get a fairly good handle on strings/pointers.

share|improve this answer

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 Jan 22 at 18:15
    
@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 Jan 22 at 18:47

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

protected by Community Apr 22 at 3:00

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

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