Is this right approach to write the code of strstr through pointers in c??

Question

char* fstrstr(char *s1,char *s2)
{
    int i=0,flag=0;
    char *s4,*s3;
//  s4 for retaining the value of s2
    s4 = s2;
    while(*s1 != '\0' && *s2 != '\0')
    {
        if(*s1 == *s2)
        {
            *(s3+i) = *s1;
            s2++;
            s1++;
            i++;
            flag = 1;
        }
        else
        {
            i = 0;
            s1++;
//          Initialize s2 again from its address
            s2 = s4;
            flag = 0;
        }
    }
    if(flag == 1)
    {
        while(*s1 != '\0')
        {
            *(s3+i) = *s1;
            i++;
            s1++;
        }
        *(s3+i) = '\0';
    }
    if(flag == 1)
        return (s3);

    if(flag==0)
    {
        *s3 = NULL;
        return (s3);
    }
}

Answer 1

I tried this function in a code and it crashes.

One issue is the line:

*(s3+i) = *s1;

Here you are assigning value to a garbage location since s3 hasn't been allocated any memory.

Answer 2

The right approach is to construct your test cases (preferably first in my opinion but that's not absolutely necessary). Create unit tests for successful and unsuccessful cases, including any tricky edge cases.

And another suggestion is to use decent variable names. Verbosity doesn't make your compiled code any slower but it does make it far more readable and maintainable.

I'd probably also use indexes instead of pointers. Not because I don't understand them, just because I often find they assist in the readability, and decent compilers will generate the same code under the covers.

Suggested test cases (for a start):

• successful find at start of string.
• successful find in middle of string.
• successful find at end of string.
• string not found.
• most of string found.
• passing NULL as either argument.
• empty strings as arguments.

---

From the looks of your code, it appears you're trying to copy the string to another location (possibly null-terminated), then return the address of that. That's not actually how the ISO strstr works. It simply returns the location of the first byte in the content string that matches the search string.

So a simplistic implementation (minimal optimisation even though a decent compiler would probably handle most of it anyway) could be as simple as:

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

char * fstrstr (char *needle, char *haystack) {
    int nlen, npos, hlen, hpos, hpos2;

    // If length of needle is 0, it's defined as found at start.

    if (*needle == 0) {
        return haystack;
    }

    // Ensure strlen only calculated once.

    nlen = strlen (needle);
    hlen = strlen (haystack);

    // Check every possible position in haystack.

    for (hpos = 0; hpos < hlen - nlen+1; hpos++) {
        // Check each character, ensuring common subexpression elimination.

        for (npos = 0, hpos2 = hpos; npos < nlen; npos++, hpos2++) {
            // No match, break early.

            if (needle[npos] != haystack[hpos2]) {
                break;
            }
        }

        // No early break, we found a match.

        if (npos == nlen) {
            return &(haystack[hpos]);
        }
    }

    // No match anywhere, return NULL.

    return NULL;
}

static char *xlat(char *p) {
    if (p == NULL) return "NULL";
    return p;
}

int main (void) {
    printf ("%s\n", xlat(fstrstr ("hel","hello world")));
    printf ("%s\n", xlat(fstrstr ("el","hello world")));
    printf ("%s\n", xlat(fstrstr ("orl","hello world")));
    printf ("%s\n", xlat(fstrstr ("rld","hello world")));
    printf ("%s\n", xlat(fstrstr ("d","hello world")));
    printf ("%s\n", xlat(fstrstr ("","hello world")));
    printf ("%s\n", xlat(fstrstr ("xyz","hello world")));
    printf ("%s\n", xlat(fstrstr ("xyz","")));
    return 0;
}

which outputs:

hello world
ello world
orld
rld
d
hello world
NULL
NULL

Note that this includes some unit tests, I haven't thoroughly tested it but it should be a good starting point.

Answer 3

You don't initialize s3 so the result of *(s3+i) = *s1 is undefined (read: most likely a crash).

Answer 4

I would say no, it's not the right way. Not only does it have major bugs (like the use of s3 which others have mentioned), but it's also

1. much longer and more complicated, with over twice the number of variables it needs, for implementing this particular string-search algorithm.
2. a very very sub-optimal algorithm, O(nm) where n is the length of the string to be searched and m is the length of the pattern string.

Here is a good reference for string search algorithms:

http://www-igm.univ-mlv.fr/~lecroq/string/index.html

Unless you're going to use one of the high-performance ones that's particularly suited to your dataset, it's probably a mistake not to just use the standard library's strstr function.