Find the rightmost occurrence of a string t, in a string s

Question

Corrected code:

int strrindex(char *s, char *t) {
  int i, j, k, p;

  i = -1;
  for (k = 0; s[k] != '\0'; k++) {
    if (strlen(s) < strlen(t)) break;
    if (s[k] == t[0]) {
      for (p = k; s[p] != '\0'; p++) {
    j = p;
        while (s[j] != '\0' && s[j] == t[j-k] && t[j-k] != '\0') { j++; } 
        if (t[j-k] != '\0') { break; }
    i = k;
      }
    }
  }

  printf("%d\n", i);
  return 0;
}

Step one: choose better variable names. I recommend at *least* two characters, maybe even three. :) — Michael Myers, Jul 04 '10 at 05:09
possible duplicate of [Is there a reverse fn() for strstr](http://stackoverflow.com/questions/1634359/is-there-a-reverse-fn-for-strstr) — Joshua, Jul 04 '10 at 05:41

score 1 · Answer 1 · answered Jul 04 '10 at 05:13

1

  for (p = k; s[p] != '\0'; p++) {
    while (s[p] == t[p] && t[p] != '\0') { p++; }

At this point, you are comparing string s starting at position p with string t starting at position 0. So it shouldn't be t[p], but instead t[p - k].

    if (s[p] != t[p] || t[p] != '\0') { i = -1; }

You don't need this at all - if the current section doesn't match, you just leave i at the last match. (which will anyway be -1 if there has been no match so far)

answered Jul 04 '10 at 05:13

casablanca

69,683
7
133
150

You don't need that long `if` statement. Try this instead: `if (t[p-k] == '\0') i = k;` - this basically accepts the current substring if all characters matched. – casablanca Jul 04 '10 at 05:27

score 0 · Answer 2 · answered Jul 04 '10 at 05:10

0

I think this should work:

 int strrindex(char *s, char *t) {

  int i = -1;
  int j = 0;
  int k = 0;
  for (; s[k]; k++) {
    if(!t[j]) {
      i = k - j;
      j = 0;
    }
    if (s[k] != t[j])
      j = 0;
    else
      j++;
  }
  if(!t[j + 1])
    i = k - j - 1;

  return i;
}

answered Jul 04 '10 at 05:10

Janick Bernet

20,544
2
29
55

It may not be the final answer, as I didn't test it (may have off by one errors...). But it's an alternative approach using only one loop :) – Janick Bernet Jul 04 '10 at 05:18
Hehe. We all started like that :) Next time you do something similar, it may only take you a few hours ;) Also, as other people suggested, try using more descriptive variable names (I'm no good role model there...) And I really doubt it will work for any border case (i.e. did you try with the strings being the same?). – Janick Bernet Jul 04 '10 at 05:25

score 0 · Answer 3 · answered Jul 04 '10 at 05:10

0

Your loop over p is indexing both s[p] and t[p]. That's incorrect, since they're both different indices.
Your if (s[p] != t[p] ...) i = -1; has no effect, because you then immediately set it to k.

answered Jul 04 '10 at 05:10

Stephen

47,994
7
61
70

Will modify accordingly and report back. Thanks – lame Jul 04 '10 at 05:16

score 0 · Answer 4 · edited May 23 '17 at 12:03

0

You're not the first person to want such a function.

See: Is there a reverse fn() for strstr? for some example implementations.

edited May 23 '17 at 12:03

Community

1
1

answered Jul 04 '10 at 05:15

fmark

57,259
27
100
107

Owen S. · Answer 5 · 2010-07-04T05:47:10.350

There are several bugs in your code. Here are the ones I've found:

You're using p, which is way into s, as an index into the beginning of t. You want to reference t[p-k] instead.
Using p in the inner while loop interferes with the operation of p in the for loop. In fact, you don't need the for loop on p, since the outer for loop takes care of iterating through s.
The while loop doesn't have any guard for when you reach the end of s, so it's pretty easy to run off of the end of s (have a partially matched t at the end of s).
(s[p] != t[p-k] || t[p-k] != '\0') will evaluate to true when t[p-k] == '\0', which is wrong – it's OK if s keeps going after t stops. You can simplify this to just (t[p-k] != '\0').
Setting i to -1 will wipe out any previous find you've made. This is wrong – an invalid match should not invalidate a previous valid match. If you get rid of the extra inner for loop, this can just turn into a continue statement.
i=k needs an else clause so it doesn't wipe out i=-1, or i=-1 needs to break/continue out of the current iteration.

1) Just noted that the inner for loop is unnecessary (modified answer), 2) putting j = p into the test is IMO bad style. — Owen S., Jul 04 '10 at 05:49

score 0 · Answer 6 · answered Jul 04 '10 at 05:30

0

There's a great reference on substring search algorithms here: http://www-igm.univ-mlv.fr/~lecroq/string/index.html

With any of them, you can simply switch things around to start with pointers to the last byte of the string and negate the indices.

answered Jul 04 '10 at 05:30

R.. GitHub STOP HELPING ICE

208,859
35
376
711

Find the rightmost occurrence of a string t, in a string s

6 Answers6