current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I am starting to write some functions while studying how to program in C. In this small function I take a string as an argument and print it out in reverse order.

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

int main(int argc, char *argv[]) {

    int aLen = argc;
    char *sWord = argv[aLen - 1];
    char *reversed;

    int sLen = strlen(sWord);
    int i = 0;

    reversed = reversed + sLen;    // offset pointer to the length of string
    while (i <= sLen) {
        if (sWord[i] != '\0') {    // check if character is null-terminator
            *reversed = sWord[i];  // if not, let reversed correct address 
        }                          // equal letter.
        i++;
        reversed--;
    }
    printf("%s reversed is:%s\n", sWord, reversed);
    return 0;
}

I was hoping to get some tips on best practice or corrections on any errors.

share|improve this question
add comment

3 Answers

up vote 5 down vote accepted

Your code has a number of issues, the main one being that it doesn't work.

If you want to reverse a string in place then I suggest you write a function just for that, separate from main. The function will need a temporary variable and will, for example:

swap first and last characters
swap 2nd and 2nd-from-last characters
swap 3rd and 3rd-from-last characters
etc

To swap the characters pointed to by a and b, you would do

char temp = *a;
*a = *b;
*b = *temp;

or if you have a pointer and a length:

char temp = a[0];
a[0] = b[len];
b[len] = temp;

So your reversal function needs to contain one of these sequences in a loop and the loop must arrange a and b to point to the correct places. For example, here is a reverse function:

static char* reverse(char *str)
{
    char *s = str;
    char *end = s + strlen(s) - 1;
    for (; s < end; ++s, --end) {
        char ch = *end;
        *end = *s;
        *s = ch;
    }
    return str;
}

Then in main all you do is call the reverse function with the desired string:

printf("'%s'\n", reverse(argv[1]));

Of course it is best to check that argv[1] exists first (remember that argv[0] is the program name and argc is always greater than or equal to 1):

if (argc == 2) {
    printf("'%s' reversed is ", argv[1]);
    printf("'%s'\n", reverse(argv[1]));
}

Notes:

In your code, aLen and sWord are unnecessary. And reversed is not initilaised but is then used in 

reversed = reversed + sLen;  // See the EDIT below

In the line

int sLen = strlen(sWord);

strlen returns the length as a size_t, not int

EDIT

When you declare a variable local to a function, the compiler sets aside a space on the stack. It does not write anything there unless you declare the initial value for the variable. So in your code, reserved contains whatever happens to be at that location on the stack and, as reserved is a pointer, it therefore points to some unknown (unplanned) location in memory. If you are unlucky that location will be writeable, your accesses 'work' and you won't see the problem (or not immediately - in a larger program you will probably encounter strange behaviour later on while the code is executing because your accesses have overwritten something important); if you are lucky and the location is unwritable you will see the problem the first time you run the program.

The moral is always to initialise variables, just to be safe. Zero (NULL) is a good value for pointers. With C99 you are not restricted to defining them at the start of a function, so it is often best to define a variable only at the point where you need it and have a useful value with which to initialise it.

share|improve this answer
    
This code compiles without errors and prints out the argument to the screen in reverse order. In what way do you mean it doesn't work? –  tijko Jun 5 '13 at 18:10
    
It compiles without errors but not without warnings - the inconsequential type conversion of strlen() and the fatal use of the uninitialised variable reversed. It cannot possibly 'work' reliably with that uninitialised access. When I run it, it seg-faults, which is not at all surprising. –  William Morris Jun 6 '13 at 0:52
    
Thanks for getting back to me on this. I didn't want to come off as disrespectful or ungrateful, considering you took the time to write a lengthy and extensive response. I haven't had this segfault on me or spit out warnings when compiling. This is why I was wanting to know exactly where and why this would fail. I have more than alot to learn with C and will be studying up on the issues you brought up here. Maybe when I make some more well informed edits I'll post again :) –  tijko Jun 6 '13 at 2:13
    
Well, I went back to testing this code and sure enough it just to fail with any string over 9 characters in length. I still don't receive any compilation warnings/errors had I, I wouldn't have even bothered with posting this. Embarassing but, I should learn plenty from your post and the others. –  tijko Jun 6 '13 at 2:39
    
It depends upon the compiler. I tried gcc and it doesn't give me a warning; clang with -Wall does. clang is often better than gcc in this respect. I will update my answer to give a little more detail on the error... –  William Morris Jun 6 '13 at 13:42
add comment
up vote 2 down vote

For simply reversing the string, you could do it without allocating additional memory:

int main(int argc, char* argv[])
{
    if(argc==2){
        char *message=argv[argc-1];
        size_t offsetLength=strlen(message);
        char *last=message+(offsetLength);
        while(last-- > message){
            putc(*(last), stdout);
        }
    }
    return 0;
}
share|improve this answer
    
Is allocating memory malloc only for resource heavy situations? –  tijko Jun 5 '13 at 2:44
    
You can allocate memory in the heap using malloc or on the stack by declaring a sized array. It doesn't need to involve "resource heavy situations." You can allocate a single byte on the stack using byte b (if the compiler doesn't inline it into a register). –  David Harkness Jun 5 '13 at 7:53
    
There is one clear answer: it depends ;) No, it is not only for ressource-heavy-situations. Yes, in some use-cases as the one above, there is no need for allocating extra memory. To make it a bit clearer: if you want to preserve the original and work with a copy, it makes sense to dynamically allocate memory needed. –  Thomas Junk Jun 5 '13 at 11:34
add comment
up vote 3 down vote

You need to allocate storage for the reversed string using malloc unless that's changed in the last twenty years and place a '\0' string terminator at the end.

char *reversed = malloc(sLen + 1);
reversed[sLen] = '\0';

Also, you're writing to cells 1 .. sLen when you should be writing to 0 .. sLen - 1.

share|improve this answer
    
I will check on the these points you brought up, I appreciate the help. –  tijko Jun 4 '13 at 21:27
    
You don't need to allocate any memory to do an in-place reversal of a string - and it looks kind of like that's what's being attempted. –  asveikau Jun 5 '13 at 7:31
    
@asveikau - I thought so as well until I saw this: reversed = reversed + sLen;. –  David Harkness Jun 5 '13 at 7:50
add comment

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.