program to reverse a string

[onus]

I am writing a program to reverse a string without using string functions available from standard library.
Following code is giving me some error when compiling

#include<stdio.h>
int main ()
{
        int i,j;
        char *p,*s,*g;
         p="abcdefgt";
         i=0;j=0;
         s=NULL;
         g=NULL;
                while (p[i] != '\0')
                {
                printf(" %d  %c\n",i,p[i]);
                i++;
                }
                i--;
          //  printf("\n outside while loop i=%d",i);        
               while (i>=0){
        //      printf("\n in while p[i]=%c s[%d]=%c\n",p[i],j,s[j]);
                 s[j]=p[i];
                 j++;
                i--;
                printf("\n j=%d i=%d",j,i);
                }
  printf(" %s\n",s);
}

I used gdb and found the error in above code was coming in

s[j]=p[i];

Why is that error coming?

[Narue]

s is a null pointer. Your code is actually pretty horrible, but the null pointer dereference is your immediate problem.

[N1GHTS]

I know I'm supposed to help you with your code, but the extreme inefficiency was killing me. A procedure like reversing a string can be used in many recursive situations and therefore must be as fast as possible. So here is some code I made which takes a string and reverses it as quickly as possible. It doesn't exactly help you fix your code, but at least you can learn a thing or two from it.

I tested it on GCC just to be sure it worked.

void ReverseString(char *String) {

  int  length     = strlen(String) - 1,
       halflength = (length / 2) + 1,
       t          = 0;
  char c;

  for (;t < halflength; t++) {
    c                = String[length-t];
    String[length-t] = String[t];
    String[t]        = c;
  }

}

P.S. I can think of a million ways to make this code even faster, but in most cases this is good enough.

[onus]

Thanks both I am clear with the problem.Actually I was asked to write without using any stdlib functions hence that logic came to my mind.I am going through your code with a pen and paper and trying to understand how is it working.

[N1GHTS]

This code swaps the first half of the characters with the last half in the same string. Since that process requires that one of the two characters are overwritten at any given time, a temp character is used to remember what was lost so it can be copied.

void ReverseString(char *String) {

  // Get the length of the string, minus
  // the final '\0' character
  int  length     = strlen(String) - 1,
  
  // Get half the length of the string
  // because we only to loop half way
  // into the string. The +1 is to
  // deal with strings with an odd number
  // length.
       halflength = (length / 2) + 1,

  // This is our iterator index
       t          = 0;

  // This is a temp character we will use
  // to temporarily copy a character we
  // are switching during the reversing
  // process.
  char c;

  // Loop only half way into the string
  for (;t < halflength; t++) {

    // Temporarily save the LAST character
    // in the string.
    c                = String[length-t];

    // Overwrite the last character in the
    // string with the first character in
    // the string.
    String[length-t] = String[t];

    // Overwrite the first character in the
    // string with the last character we saved
    // in this temp character.
    String[t]        = c;
  }

}

For instance...

[H][E][L][L][O][\0] (Length = 6)

We will not move '\0' so we subtract Length by 1. So it will kinda look like this to our program:

[H][E][L][L][O] (Length = 5)

Our Half length is going to be 2.5, but it gets chopped off since its an int. So it ends up being 2. Now we add that by 1. So the half length is 3.

First the loop will remember the last character...

c = [O]

Then the loop will overwrite the end character...

[H][E][L][L][H]
\__________/|

Now it takes the [O] it remembered and copies it to the front...

[O][E][L][L][H]

So it swapped them. The loop continues by shrinking the swap area...

[O][E][L][L][H]
.....\_____/.....

And after going through the process of switching, it ends up with

[O][L][L][E][H]

The last iteration is actually useless, and may not be required, but
I left it in there just in case...

[O][L][L][E][H]
........\_/........

It doesn't hurt to copy itself to itself, but if you wanted to refine
this function for speed maybe double check my math and see where it
can be improved.