HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Learning C, basic string reversal function

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
functionlearningreversalstringbasic

Problem

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 
#include 

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.

Solution

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.

Code Snippets

swap first and last characters
swap 2nd and 2nd-from-last characters
swap 3rd and 3rd-from-last characters
etc
char temp = *a;
*a = *b;
*b = *temp;
char temp = a[0];
a[0] = b[len];
b[len] = temp;
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;
}
printf("'%s'\n", reverse(argv[1]));

Context

StackExchange Code Review Q#27003, answer score: 5

Revisions (0)

No revisions yet.