From: dave@cs.arizona.edu (Dave Schaumann)
Newsgroups: comp.lang.c
Subject: Re: HIgher problems at hand. HELP!
Date: 24 Jul 92 01:02:54 GMT
Reply-To: dave@cs.arizona.edu (Dave Schaumann)
Organization: University of Arizona

In article <Brv6oD.Dq5@usenet.ucs.indiana.edu>, shulick@navajo (Sam Hulick) writes:
>What's wrong with this subroutine?

Since you asked...

>int strstr(char *data, char *chunk) 
                  ^^^^        ^^^^^
I'm not thrilled with your variable names.  "text" and "pattern", I think,
would've been better choices.

>        static int x;                                        
         ^^^^^^
Why make x static?  It serves no purpose.  (Unless statically allocated
variables run significantly faster on your compiler.  Certainly the 68000
has enough registers that this variable could easily be put in one)

>        for (x=0;x<strlen(data); ++x)   {                    
                  ^^^^^^^^^^^^^^
A little white space goes a long way toward readability.  Also, strlen()
is needlessly wasteful here.  This is better writtin, I think, as

         for( x = 0 ; data[x] != '\0' ; x++ ) {

Better yet, you could eliminate x all together:

	for( ; *data ; data++ )

(of course, making the necessary changes in the loop body)

>                if (!strncmp(chunk, &data[x], strlen(chunk)))
                     ^^^^^^^^                  ^^^^^^
I'm not too fond of using a boolean operator on strncmp(), since it returns
3 value ranges, and not two.  And again, you have another call to strlen
inside your loop.  While this one appears to be necessary, you should've
called this once outside the loop, and then stored the value in a variable.

>                        return x+1;                          
                                ^^^
x+1?  At this point, x is the index of the matched pattern in the text.  You
should return x here.

Also, according to my manpage, 

     char *strstr( char *s1, char *s2 )

     strstr() returns a pointer to the first  occurrence  of  the
     pattern  string  s2  in  s1.   For example, if s1 is "string
     thing" and s2 is "ing", strstr() returns "ing thing".  If s2
     does not occur in s1, strstr() returns NULL.

From your statement
> d  = strstr("foobar", "bar"); and d comes up like 139579275

I would say that your library /does/ have strstr(), but it's not doing
what you expect.  Try

	printf( "%s\n", strstr( "foobar", "bar" ) ;

and see what happens.

If you really want a routine that does what you described, try this:

int find( char *text, char *pattern ) {

  /* Depending on how good your compiler is at optimization, you may */
  /* want to use the `register' modifier on t's declaration. */

  char *t ;
  int plen = strlen(pattern) ;
  for( t = text ; *t ; t++ )
    if( strncmp(t, pattern, plen) == 0 )
      return t - text ;
  return -1 ;
  }

This still has its problems (most notably, it will continue to loop even when
pattern is longer than t), but it should be significantly faster than yours,
and IMHO it's a lot clearer too.

-- 
Mary had a swingin' lamb/he followed her to
	school

She hocked his wool for a bongo drum and man that lamb was
	cool.

		-Mr. Know-it-all
Newsgroups: comp.lang.c,comp.sys.amiga.programmer