C problem inside function that searches a word inside another one and returns position

The following C problem searches s1 inside s2 and returns the position that s1 was found in s2. i wrote this code and it works fine for values like s1: car s2: carnal but if i have s1:car and s2: cbrcarnal i think it enters an infinite loop or smth but it won't display nothing.Can you guys see the problem? it must be in my function. Oh and I'm not allowed to use strstr.

code:

#include "stdafx.h"
#i开发者_运维技巧nclude "stdio.h"
#include "string.h"

int subsir(char s1[],char s2[],int k)
{    int n,m,i=0,j,poz=-1;
n=strlen(s1);
m=strlen(s2);
if(n>m)
return -1;
j=k;
while(j<=m-n)
if((s1[i]==s2[j])&&(s1[n-1]==s2[j+n-1]))
{
    poz=j;
    while(j+1<poz+n-1)
        if(s1[i+1]==s2[j+1])
            {i++;
            j++;
            }
        else
            return subsir(s1,s2,poz++);
}
else
j++;

if(poz!=-1)
    return poz;
else
    return -1;
}

int _tmain(int argc, _TCHAR* argv[])
{char s1[30],s2[30];
int n;
printf("introduceti sirul 1: ");
scanf("%s",&s1);
printf("introduceti sirul 2: ");
scanf("%s",&s2);
n=subsir(s1,s2,0);
if(n!=-1)
    printf("%s is found in %s, at: %d\n",s1,s2,n);
else
    printf("s %s is not found in %s\n",s1,s2);

}

Here is a formatted version of your code. The only changes I made were to comments, braces, whitespace, declaring variables closer to where they're used, changing one while loop into a for loop, and removing the ineffective post-increment. It does exactly the same as your code:

int subsir(char s1[], char s2[], int k) {
    // Return location of s1 in s2, starting at index k in s2.

    int n = strlen(s1);
    int m = strlen(s2);
    if (n > m)  // s1 cannot be in s2
        return -1;

    int poz = -1;
    for (int j = k, i = 0; j <= m - n;) {
        // if first and last character in s1 match corresponding locations in s2
        if((s1[i] == s2[j]) && (s1[n - 1] == s2[j + n - 1])) {
            // save current s2 location in poz
            poz = j;
            while (j + 1 < poz + n - 1) {
                if(s1[i + 1] == s2[j + 1]) {
                    i++;
                    j++;
                }
                else {
                    return subsir(s1, s2, poz);
                }
            }
        }
        else {
            j++;
        }
    }

    return poz;
}

With this version it should be much easier for you to spot problems:

• I know you had it in mind and either written down somewhere explicitly (or you were just reading from the assignment description), but it always helps your thought process to include a short sentence about the purpose of a function. This is the first comment.

• I would not normally include the second and third comments in my code, but I'm writing for a different audience than you are. Explaining "why" (for the second comment) and how more complicated expressions work (for the third comment) should help you.

• The forth comment is a start at specifying the role poz serves. You should be able to state, similar to the short function description, a short blurb about each variable. Usually, such as for variables n, m, k, j, and i, you don't have to put this explicitly in the code — but if you get confused, start adding them. For example, "k is the starting index to search", "n is the length of s1", "j and i are the 'current' locations in s2 and s1, respectively", and so on.

• The post-increment I removed was in a return statement, but since control never returns to this function (and poz is thus never used again), it has no affect.

• You checked if poz was -1, and if it was, returned -1; otherwise returning poz. This is the same as returning poz directly.

• You modify i, but if control escapes that while loop, you never reset it to zero; thus, you never start searching from the beginning of s1 from then on.

• Because you start with poz (instead of poz + 1) on the recursive call, you get stuck in a loop continually checking the same location over and over again. (Passing poz + 1 fixes this.)

To continue from here, I'd refactor out your inner loop into a separate function doing the equivalent of strncmp ­— or use strncmp directly, if you can. Greater modularity makes problems easier to reason about.

Recursion is not necessary here, but it's fine to model the problem that way:

• if s1 is in s2 starting at position k, return k
• else call subsir(s1, s2, k + 1) to search at the next location
  • this would normally be done in a loop rather than a recursive call, but with tail-call optimization, it's exactly the same

Your code is sooo complicated... I honestly don't feel like looking at it closely. But I would like to show you how I would do it

int search(char* what, char* where)
{
   int whatlen = strlen(what);
   int wherelen = strlen(where);
   for(int i = 0; i <= wherelen - whatlen; ++i)
   {
      int j;
      for(j = 0; j < whatlen; ++j)
      {
          if(what[j] != where[i+j])
             break;
      }
      if(j == whatlen)
         return i;
   }
   return -1;
}

I think the problem is that you're not using strstr.